Hi Tyler,
Richard already gave a pretty thorough review, but I have a few other questions:
> I think it makes sense because c++11 attributes, for example [[loop
> vectorize(4)]], would be verified at this point too.
Why is that? Shouldn’t they both be dealt with in Sema? I don’t like the idea
that I wouldn’t get these semantic diagnostics with -fsyntax-only.
> Index: test/Parser/pragma-loop2.cpp
> ===================================================================
> --- test/Parser/pragma-loop2.cpp (revision 0)
> +++ test/Parser/pragma-loop2.cpp (working copy)
> @@ -0,0 +1,50 @@
> +// RUN: not %clang %s 2>&1 | FileCheck %s
This test just highlights why this should be done in Sema if at all possible…
Also, you probably don’t need the driver here.
> + // Add vectorize hints to the metadata on the conditional branch.
> + // Iterate in reverse so hints are put in the same order they appear.
> + SmallVector<llvm::Value*, 2> Metadata(1);
> + ArrayRef<const Attr*>::reverse_iterator I;
> + for (I = Attrs.rbegin(); I != Attrs.rend(); ++I) {
> + const LoopHintAttr *LH = cast<LoopHintAttr>(*I);
Shouldn’t this dyn_cast and continue on a nullptr result? What happens when I
add another attribute?
> +def warn_pragma_loop_invalid_option : Warning<
> + "invalid option '%0' in directive '#pragma loop', expected either
> vectorize or interleave - ignoring">,
> + InGroup<PragmaLoop>;
In this and the other diagnostics I think we want a semicolon before “expected”
rather than a comma. Why are all the diagnostics warnings instead of errors?
Ben
On May 6, 2014, at 4:36 PM, Richard Smith <[email protected]> wrote:
> This still needs an explicit LGTM from a committer, per the developer policy.
>
> On Tue, May 6, 2014 at 4:23 PM, Nadav Rotem <[email protected]> wrote:
> I think that Tyler addressed all of the comments that the reviewers had, so
> unless there are any other objections I think this patch should be committed.
>
> On May 6, 2014, at 4:22 PM, Arnold Schwaighofer <[email protected]>
> wrote:
>
>> It seems to me that the patch is in pretty good shape. Does it make sense do
>> the rest of the review in tree?
>>
>>
>> On May 6, 2014, at 1:19 PM, Tyler Nowicki <[email protected]> wrote:
>>
>>> Hi Alexey,
>>>
>>> Thanks for the review. I applied many of your suggestions. Please review my
>>> comments below. Here is the updated patch.
>>>
>>> Tyler
>>>
>>> <pragma_loop-svn.patch>
>>>
>>>
>>>> + if (Type == LoopHintAttr::Value) {
>>>> + llvm::APSInt ValueAPS;
>>>> + if(!ValueExpr || !ValueExpr->EvaluateAsInt(ValueAPS,
>>>> CGM.getContext()) ||
>>>> + (ValueInt = ValueAPS.getSExtValue()) < 1) {
>>>> + CGM.getDiags().Report(LH->getRange().getEnd(),
>>>> + diag::warn_pragma_loop_invalid_value) <<
>>>> + LH->getSpelling();
>>>> + continue;
>>>> + }
>>>> + }
>>>> +
>>>> + llvm::Value *Value;
>>>> + llvm::MDString *Name;
>>>> + LoopHintAttr::Spelling S = LH->getSemanticSpelling();
>>>> +
>>>> + if (S == LoopHintAttr::Keyword_vectorize) {
>>>> + // Do not add hint if it is incompatible with prior hints.
>>>> + if (!LoopHintAttr::isCompatible(VectorizeState | Type)) {
>>>> + CGM.getDiags().Report(LH->getRange().getEnd(),
>>>> + diag::warn_pragma_loop_incompatible) <<
>>>> + LoopHintAttr::getName(VectorizeState) <<
>>>> + LoopHintAttr::getName(Type) <<
>>>> + LH->getSpelling();
>>>> + continue;
>>>> + }
>>>>
>>>> + } else if (S == LoopHintAttr::Keyword_interleave) {
>>>> + // Do not add hint if it is incompatible with prior hints.
>>>> + if (!LoopHintAttr::isCompatible(InterleaveState | Type)) {
>>>> + CGM.getDiags().Report(LH->getRange().getEnd(),
>>>> + diag::warn_pragma_loop_incompatible) <<
>>>> + LoopHintAttr::getName(InterleaveState) <<
>>>> + LoopHintAttr::getName(Type) <<
>>>> + LH->getSpelling();
>>>> + continue;
>>>> + }
>>>>
>>>>
>>>> I think it should be verified in Sema, not in CodeGen
>>>
>>> I think it makes sense because c++11 attributes, for example [[loop
>>> vectorize(4)]], would be verified at this point too.
>>>
>>>
>>>> 4. lib/Parse/ParsePragma.cpp
>>>>
>>>> BalancedDelimiterTracker is not used for parsing.
>>>
>>> I looked at using this. BDT requires an instance of Parser which is not
>>> given to the pragma handlers (see initializePragmaHandlers). No other
>>> pragmas use BDT. If you think pragmas should use BDT then it should be done
>>> in another patch.
>>>
>>>
>>>> + case tok::annot_pragma_loop_hint:
>>>> + ProhibitAttributes(Attrs);
>>>> + return ParsePragmaLoopHint(Stmts, OnlyStatement, TrailingElseLoc,
>>>> Attrs);
>>>>
>>>> Why attributes are prohibited?
>>>
>>> ProhibitAttributes informs the programmer attributes are not allowed if any
>>> are given. I don’t think attributes are allowed on preprocessor directives
>>> ([[attribute]] #pragma …?) and none of the existing pragmas allow
>>> attributes.
>>>
>>>
>>>> + if (Tok.is(tok::kw___attribute) &&
>>>> + (NextTok.is(tok::kw_do) ||
>>>> + NextTok.is(tok::kw_for) ||
>>>> + NextTok.is(tok::kw_while)) ) {
>>>> + // Get the loop's attributes.
>>>> + MaybeParseCXX11Attributes(Attrs, 0, /*MightBeObjCMessageSend*/ true);
>>>> + }
>>>>
>>>> I don't think that this correct to handle attributed statements. C++11
>>>> does not use __attribute as a key word for attributes, but '[[' sequence.
>>>> I think it would be better just to call MaybeParseCXX11Attributes(...)
>>>> without any preconditions. Besides, AttributedStmt will be never created,
>>>> because you don't call Sema::ProcessStmtAttributes(...) after all.
>>>
>>> You are right, I improved this part of the code. But I also think you
>>> missed something important about how this works. The pragma for loop hint
>>> is turned into an Attrs of an AttributedStmt, but this does not happen
>>> right away. The loop hint gets added to a ParsedAttributes list. If the
>>> subsequent loop also has attributes those are also added to the
>>> ParsedAttributes list. ParsePragmaLoopHint returns a loop stmt and the
>>> ParsedAttributes list. Both are turned into an AttributedStmt by the call
>>> to ProcessStmtAttributes by ParseStatementOrDeclaration.
>>>
>>>
>>>> 6. lib/Sema/SemaStmtAttr.cpp
>>>>
>>>> + if (St->getStmtClass() != Stmt::DoStmtClass &&
>>>> + St->getStmtClass() != Stmt::ForStmtClass &&
>>>> + St->getStmtClass() != Stmt::CXXForRangeStmtClass &&
>>>> + St->getStmtClass() != Stmt::WhileStmtClass) {
>>>> + S.Diag(St->getLocStart(), diag::warn_pragma_loop_precedes_nonloop);
>>>> + return 0;
>>>> + }
>>>>
>>>> AttributedStmts are not allowed?
>>>
>>> No, this function examines the loop hint ParsedAttribute and returns an
>>> Attr. The result of ProcessStmtAttributes is an AttributedStmt.
>>> _______________________________________________
>>> cfe-commits mailing list
>>> [email protected]
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>
>
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
