<div dir="ltr"><div>+ // This attribute has no spellings. It is</div><div>+ // created when pragma loop is specified.</div><div>+ let Spellings = [Keyword<"vectorize">,</div><div>+ Keyword<"interleave">];</div>
<div><br></div><div>If it has no spellings, you should not list any spellings. You can't spell this as a keyword, so listing these spellings seems incorrect.</div><div><br></div><div>Ultimately, the right thing to do here is probably to add a new flavour of spelling, Pragma<...>.</div>
<div><br></div><div><div>+ let Args = [IntArgument<"type">,</div><div><br></div><div>"type" is a bad name to use for anything other than a type. We usually use "kind" for this.</div><div>
<br></div><div>+ ExprArgument<"value">];</div><div><br></div><div>As far as I can see, is as an optional ExprArgument (ExprArgument<"value", 1>). Any reason not to model it as one?</div>
<div><br></div><div>+ let AdditionalMembers = [{</div><div>+ enum Type {</div><div>+ Unknown = 0,</div><div>+ Enable = 1,</div><div>+ Disable = 2,</div><div>+ Value = 4</div><div>+ };</div></div><div><br>
</div><div>This looks a bit weird. Why do we have flags for both 'Enable' and 'Disable'? These should at least have comments. I assume they mean something like 'try hard to vectorize' and 'never vectorize', with the default being 'if the optimizer feels like it'?</div>
<div><br></div><div><div>+ static bool isCompatible(unsigned State) {</div><div>+ return State != (Disable | Enable) &&</div><div>+ State != (Disable | Value) &&</div><div>+ State != (Disable | Enable | Value);</div>
<div>+ }</div></div><div><br></div><div>I'd find this a lot clearer as:</div><div><br></div><div>if (State & Disable)</div><div> return State == Disable;</div><div>return true;</div><div><br></div><div><br></div>
<div><div>+// Pragma loop support.</div><div>+def warn_pragma_loop_invalid_option : Warning<</div><div>+ "invalid option '%0' in directive '#pragma loop', expected either vectorize or interleave - ignoring">,</div>
<div>+ InGroup<PragmaLoop>;</div><div>+def warn_pragma_loop_missing_option : Warning<</div><div>+ "missing option in directive '#pragma loop', expected either vectorize or interleave - ignoring">,</div>
<div>+ InGroup<PragmaLoop>;</div><div>+def warn_pragma_loop_invalid_type : Warning<</div><div>+ "invalid value '%0' in directive '#pragma loop %1', expected either 'enable', 'disable', or a positive integer - ignoring">,</div>
<div>+ InGroup<PragmaLoop>;</div><div>+def warn_pragma_loop_precedes_nonloop : Warning<</div><div>+ "expected a for, while, or do-while loop to follow the '#pragma loop' directive - ignoring">,</div>
<div>+ InGroup<PragmaLoop>;</div></div><div><br></div><div>These should all be InGroup<IgnoredPragmas></div><div><br></div><div><div>+def warn_pragma_loop_invalid_value : Warning<</div><div>+ "expected a positive integer in directive '#pragma loop %0' - ignoring">,</div>
<div>+ InGroup<PragmaLoop>;</div><div>+def warn_pragma_loop_incompatible : Warning<</div><div>+ "'%0' and '%1' directive option types are incompatible in '#pragma loop %2' - ignoring">,</div>
<div>+ InGroup<PragmaLoop>;</div></div><div><br></div><div>Likewise.</div><div><br></div><div><div>+/// \brief Loop hint specified by a pragma loop directive</div><div>+/// and used by codegen to attach metadata to the IR.</div>
<div>+struct LoopHint {</div><div>+ SourceRange Range;</div><div>+ ExprResult ValueExpr;</div><div>+ IdentifierLoc *ValueLoc;</div><div>+ IdentifierLoc *OptionLoc;</div><div>+};</div></div><div><br></div><div>It doesn't make sense to store an ExprResult in a struct that's intended for CodeGen. This should be an Expr* if it represents a semantically-valid loop hint. Also, you can't put an ExprResult or an IdentifierLoc into Basic, that's a huge layering violation. Also, it's surprising that we're sharing a data structure between the Parser, Sema, and CodeGen. But... it looks like this comment is wrong, and this structure is only used to pass data between Parser and Sema. It should be in include/clang/Sema instead.</div>
<div><br></div><div><div>+ // FIXME: When we get more attributes which have a pragma</div><div>+ // form LoopHintAttr should be replaced with a base class</div><div>+ // for pretty printing these types of attributes.</div>
<div>+ if (const LoopHintAttr *LHA = dyn_cast<LoopHintAttr>(*it))</div><div>+ LHA->printPragma(OS, Policy);</div></div><div><br></div><div>We worked hard to get hacks like this out of the attribute handling code; it seems unfortunate to add another one. (I'm fine with you saying you're going to fix this in a follow-up commit; I'm less fine with saying this is the problem of whoever walks this path next.)</div>
<div><br></div><div><skipping IRGen changes></div><div><div><br></div><div>+/// \code</div><div>+/// #pragma loop vectorize(enable/disable/_value_)</div><div>+/// #pragma loop interleave(enable/disable/_value_)</div>
<div>+/// #pragma loop vectorize(...) interleave(...) ...</div><div>+/// \endcode</div></div><div><br></div><div><div>We normally express grammar fragments in comments in a BNF-like style. Something like:</div><div><br>
</div><div> #pragma 'loop' loop-hints</div><div><br></div><div> loop-hints:</div><div> loop-hint loop-hints[opt]</div><div><br></div><div> loop-hint:</div><div> 'vectorize' '(' loop-hint-value ')'</div>
<div> 'interleave' '(' loop-hint-value ')'</div><div><br></div><div> loop-hint-value:</div><div> 'enable'</div><div> 'disable'</div><div> constant-expression</div><div>
<br></div><div>+/// Specifying vectorize(enable) or vectorize(_value_) instructs llvm to</div><div>+/// try vectorizing the subsequent loop. Specifying interleave(enable) or</div><div>+/// interleave(_value_) instructs llvm to tru interleaving the subsequent loop</div>
</div><div><br></div><div>Typo 'tru'. Missing period.</div><div><br></div><div><div>+/// Giving a _value_ of 1 or "disable" prevents the optimization, even if it</div><div>+/// is possible or profitable. The loop vectorizer currently only works on</div>
<div>+/// inner loops.</div></div><div><br></div><div>What do other possibilities for 'value' mean? And does 1 really mean disable? (That seems backwards...) Why have 'enable' and 'disable' at all if 0 and 1 work?</div>
<div><br></div><div><div>+ PragmaLoopHintInfo *Info =</div><div>+ (PragmaLoopHintInfo*) PP.getPreprocessorAllocator().Allocate(</div><div>+ sizeof(PragmaLoopHintInfo), llvm::alignOf<PragmaLoopHintInfo>());</div>
<div>+</div><div>+ Info->Option = Tok;</div><div>+ StringRef Option = Tok.getIdentifierInfo()->getName();</div><div>+</div><div>+ if (Option != "vectorize" && Option != "interleave") {</div>
<div>+ PP.Diag(Tok.getLocation(), diag::warn_pragma_loop_invalid_option) <<</div><div>+ Option;</div><div>+ return;</div><div>+ }</div></div><div><br></div><div>Reorder this so that you don't perform the allocation if the option is invalid.</div>
<div><br></div><div><div>+ // Read '('</div><div>+ PP.Lex(Tok);</div><div>+ if (Tok.isNot(tok::l_paren)) {</div><div>+ PP.Diag(Tok.getLocation(), diag::err_expected) << tok::l_paren;</div><div>
+ return;</div><div>+ }</div><div>+</div><div>+ // Read the optimization value identifier.</div><div>+ PP.Lex(Tok);</div><div>+ if (Tok.isNot(tok::identifier) && Tok.isNot(tok::numeric_constant)) {</div>
<div>+ PP.Diag(Tok.getLocation(), diag::warn_pragma_loop_invalid_type) <<</div><div>+ Tok.getName();</div><div>+ return;</div><div>+ }</div><div>+</div><div>+ Info->Value = Tok;</div><div>+</div>
<div>+ // Read ')'</div><div>+ PP.Lex(Tok);</div><div>+ if (Tok.isNot(tok::r_paren)) {</div><div>+ PP.Diag(Tok.getLocation(), diag::err_expected) << tok::r_paren;</div><div>+ return;</div>
<div>+ }</div></div><div><br></div><div>This isn't going to work (and in fact this whole pragma parsing approach isn't going to work) once you start wanting to handle template arguments and so on. See Parser::HandlePragmaMSPragma for examples of how to do this in a way that lets you properly parse a constant expression here.</div>
<div><br></div><div>+ // Consume all remaining token in the pragam loop directive.<br></div><div><br></div><div>Typo 'pragam'.</div><div><br></div><div>+ PP.Diag(Tok.getLocation(), diag::warn_pragma_extra_tokens_at_eol) <<<br>
</div><div><br></div><div>(Here and elsewhere) << goes at the start of the line, not the end.</div><div><br></div><div><div>+ Token *TokenArray = new Token[TokenList.size()];</div><div>+ for(unsigned long i = 0; i < TokenList.size(); i++) {</div>
<div>+ TokenArray[i] = TokenList[i];</div><div>+ }</div></div><div><br></div><div>Use std::copy.</div><div><br></div><div>+ TokenList.clear();<br></div><div><br></div><div>Unnecessary; please remove.</div><div><br></div>
<div><div>+StmtResult Parser::ParsePragmaLoopHint(StmtVector &Stmts, bool OnlyStatement,</div><div>+ SourceLocation *TrailingElseLoc,</div><div>+ ParsedAttributesWithRange &Attrs)</div>
<div>+{</div><div>+ // Create temporary attribute list.</div><div>+ ParsedAttributesWithRange TempAttrs(AttrFactory);</div><div>+</div><div>+ // Get vectorize hints and consume annotated token.</div><div>+ while (Tok.is(tok::annot_pragma_loop_hint)) {</div>
<div>+ LoopHint Hint = HandlePragmaLoopHint();</div><div>+ ConsumeToken();</div><div>+</div><div>+ if (!Hint.ValueLoc || !Hint.OptionLoc)</div><div>+ continue;</div><div>+</div><div>+ ArgsUnion ArgHints[] = {Hint.ValueLoc,</div>
<div>+ ArgsUnion(Hint.ValueExpr.get())};</div><div>+ TempAttrs.addNew(Hint.OptionLoc->Ident, Hint.Range,</div><div>+ 0, Hint.OptionLoc->Loc,</div><div>+ ArgHints, 2,</div>
<div>+ AttributeList::AS_Keyword);</div><div>+ }</div><div>+</div><div>+ // Get the next statement.</div><div>+ MaybeParseCXX11Attributes(Attrs);</div><div>+</div><div>+ if (isTokenSpecial()) {</div>
<div>+ Diag(Tok.getLocation(), diag::warn_pragma_loop_precedes_nonloop);</div><div><br></div><div>This special case makes no sense.</div><div><br></div><div>+ return StmtEmpty();</div><div><br></div><div>Why is this not an error?</div>
<div><br></div><div>+ }</div><div>+</div><div>+ Token StmtTok = Tok;</div><div>+ StmtResult S = ParseStatementOrDeclarationAfterAttributes(Stmts,</div><div>+ /*OnlyStatement*/ true, 0, Attrs);</div><div>
+</div><div>+ // If it is a loop add the loop hint attributes to it.</div><div>+ if (StmtTok.is(tok::kw_for) || StmtTok.is(tok::kw_while) || StmtTok.is(tok::kw_do)) {</div><div>+ Attrs.takeAllFrom(TempAttrs);</div><div>
+ return S;</div><div>+ }</div><div>+</div><div>+ Diag(StmtTok.getLocation(), diag::warn_pragma_loop_precedes_nonloop);</div><div>+ return S;</div><div>+}</div></div><div><br></div><div>Funneling your hints through the attribute mechanism seems very strange. Instead, just parse a substatement then call a Sema function to ActOnPragmaLoopHint(LoopHint, StmtResult).</div>
</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, May 6, 2014 at 4:36 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra">This still needs an explicit LGTM from a committer, per the developer policy.</div>
<div><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Tue, May 6, 2014 at 4:23 PM, Nadav Rotem <span dir="ltr"><<a href="mailto:nrotem@apple.com" target="_blank">nrotem@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">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. <div>
<div><div><br><div><div>On May 6, 2014, at 4:22 PM, Arnold Schwaighofer <<a href="mailto:aschwaighofer@apple.com" target="_blank">aschwaighofer@apple.com</a>> wrote:</div><br><blockquote type="cite"><div style="word-wrap:break-word">
It seems to me that the patch is in pretty good shape. Does it make sense do the rest of the review in tree?<div><br></div><div><br><div><div>On May 6, 2014, at 1:19 PM, Tyler Nowicki <<a href="mailto:tnowicki@apple.com" target="_blank">tnowicki@apple.com</a>> wrote:</div>
<br><blockquote type="cite"><div style="word-wrap:break-word">Hi Alexey,<div><br></div><div>Thanks for the review. I applied many of your suggestions. Please review my comments below. Here is the updated patch.</div><div>
<br></div><div>Tyler</div><div><br></div><div></div></div>
<span><pragma_loop-svn.patch></span><div style="word-wrap:break-word"><div></div><div><br></div><div><br></div><div><blockquote type="cite">+ if (Type == LoopHintAttr::Value) {<br>+ llvm::APSInt ValueAPS;<br>
+ if(!ValueExpr || !ValueExpr->EvaluateAsInt(ValueAPS, CGM.getContext()) ||<br>+ (ValueInt = ValueAPS.getSExtValue()) < 1) {<br>+ CGM.getDiags().Report(LH->getRange().getEnd(),<br>+ diag::warn_pragma_loop_invalid_value) <<<br>
+ LH->getSpelling();<br>+ continue;<br>+ }<br>+ }<br>+<br>+ llvm::Value *Value;<br>+ llvm::MDString *Name;<br>+ LoopHintAttr::Spelling S = LH->getSemanticSpelling();<br>
+<br>+ if (S == LoopHintAttr::Keyword_vectorize) {<br>+ // Do not add hint if it is incompatible with prior hints.<br>+ if (!LoopHintAttr::isCompatible(VectorizeState | Type)) {<br>+ CGM.getDiags().Report(LH->getRange().getEnd(),<br>
+ diag::warn_pragma_loop_incompatible) <<<br>+ LoopHintAttr::getName(VectorizeState) <<<br>+ LoopHintAttr::getName(Type) <<<br>
+ LH->getSpelling();<br>+ continue;<br>+ }<br><br>+ } else if (S == LoopHintAttr::Keyword_interleave) {<br>+ // Do not add hint if it is incompatible with prior hints.<br>
+ if (!LoopHintAttr::isCompatible(InterleaveState | Type)) {<br>+ CGM.getDiags().Report(LH->getRange().getEnd(),<br>+ diag::warn_pragma_loop_incompatible) <<<br>+ LoopHintAttr::getName(InterleaveState) <<<br>
+ LoopHintAttr::getName(Type) <<<br>+ LH->getSpelling();<br>+ continue;<br>+ }<br><br><br>I think it should be verified in Sema, not in CodeGen<br>
</blockquote><div><br></div><div>I think it makes sense because c++11 attributes, for example [[loop vectorize(4)]], would be verified at this point too.</div><div><br></div><div><br></div><blockquote type="cite">4. lib/Parse/ParsePragma.cpp<br>
<br>BalancedDelimiterTracker is not used for parsing.<br></blockquote><div><br></div><div>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.</div>
<div><br></div><div><br></div><blockquote type="cite">+ case tok::annot_pragma_loop_hint:<br>+ ProhibitAttributes(Attrs);<br>+ return ParsePragmaLoopHint(Stmts, OnlyStatement, TrailingElseLoc, Attrs);<br><br>Why attributes are prohibited?<br>
</blockquote><div><br></div><div>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.</div>
<div><br></div><div><br></div><blockquote type="cite">+ if (<a href="http://tok.is/" target="_blank">Tok.is</a>(tok::kw___attribute) &&<br>+ (<a href="http://nexttok.is/" target="_blank">NextTok.is</a>(tok::kw_do) ||<br>
+ <a href="http://nexttok.is/" target="_blank">NextTok.is</a>(tok::kw_for) ||<br>+ <a href="http://nexttok.is/" target="_blank">NextTok.is</a>(tok::kw_while)) ) {<br>+ // Get the loop's attributes.<br>
+ MaybeParseCXX11Attributes(Attrs, 0, /*MightBeObjCMessageSend*/ true);<br>
+ }<br><br>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.<br>
</blockquote><div><br></div><div>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.</div>
<div><br></div><div><br></div><blockquote type="cite">6. lib/Sema/SemaStmtAttr.cpp<br><br>+ if (St->getStmtClass() != Stmt::DoStmtClass &&<br>+ St->getStmtClass() != Stmt::ForStmtClass &&<br>+ St->getStmtClass() != Stmt::CXXForRangeStmtClass &&<br>
+ St->getStmtClass() != Stmt::WhileStmtClass) {<br>+ S.Diag(St->getLocStart(), diag::warn_pragma_loop_precedes_nonloop);<br>+ return 0;<br>+ }<br><br>AttributedStmts are not allowed?<br></blockquote><div>
<br></div><div>No, this function examines the loop hint ParsedAttribute and returns an Attr. The result of ProcessStmtAttributes is an AttributedStmt.</div></div></div>_______________________________________________<br>cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div></div></blockquote></div><br></div></div></div></div></blockquote></div><br></div></div></div></div>
</blockquote></div><br></div>