[PATCH] #pragma vectorize
Richard Smith
richard at metafoo.co.uk
Tue May 6 17:20:04 PDT 2014
+ // This attribute has no spellings. It is
+ // created when pragma loop is specified.
+ let Spellings = [Keyword<"vectorize">,
+ Keyword<"interleave">];
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.
Ultimately, the right thing to do here is probably to add a new flavour of
spelling, Pragma<...>.
+ let Args = [IntArgument<"type">,
"type" is a bad name to use for anything other than a type. We usually use
"kind" for this.
+ ExprArgument<"value">];
As far as I can see, is as an optional ExprArgument (ExprArgument<"value",
1>). Any reason not to model it as one?
+ let AdditionalMembers = [{
+ enum Type {
+ Unknown = 0,
+ Enable = 1,
+ Disable = 2,
+ Value = 4
+ };
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'?
+ static bool isCompatible(unsigned State) {
+ return State != (Disable | Enable) &&
+ State != (Disable | Value) &&
+ State != (Disable | Enable | Value);
+ }
I'd find this a lot clearer as:
if (State & Disable)
return State == Disable;
return true;
+// Pragma loop support.
+def warn_pragma_loop_invalid_option : Warning<
+ "invalid option '%0' in directive '#pragma loop', expected either
vectorize or interleave - ignoring">,
+ InGroup<PragmaLoop>;
+def warn_pragma_loop_missing_option : Warning<
+ "missing option in directive '#pragma loop', expected either vectorize
or interleave - ignoring">,
+ InGroup<PragmaLoop>;
+def warn_pragma_loop_invalid_type : Warning<
+ "invalid value '%0' in directive '#pragma loop %1', expected either
'enable', 'disable', or a positive integer - ignoring">,
+ InGroup<PragmaLoop>;
+def warn_pragma_loop_precedes_nonloop : Warning<
+ "expected a for, while, or do-while loop to follow the '#pragma loop'
directive - ignoring">,
+ InGroup<PragmaLoop>;
These should all be InGroup<IgnoredPragmas>
+def warn_pragma_loop_invalid_value : Warning<
+ "expected a positive integer in directive '#pragma loop %0' - ignoring">,
+ InGroup<PragmaLoop>;
+def warn_pragma_loop_incompatible : Warning<
+ "'%0' and '%1' directive option types are incompatible in '#pragma loop
%2' - ignoring">,
+ InGroup<PragmaLoop>;
Likewise.
+/// \brief Loop hint specified by a pragma loop directive
+/// and used by codegen to attach metadata to the IR.
+struct LoopHint {
+ SourceRange Range;
+ ExprResult ValueExpr;
+ IdentifierLoc *ValueLoc;
+ IdentifierLoc *OptionLoc;
+};
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.
+ // FIXME: When we get more attributes which have a pragma
+ // form LoopHintAttr should be replaced with a base class
+ // for pretty printing these types of attributes.
+ if (const LoopHintAttr *LHA = dyn_cast<LoopHintAttr>(*it))
+ LHA->printPragma(OS, Policy);
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.)
<skipping IRGen changes>
+/// \code
+/// #pragma loop vectorize(enable/disable/_value_)
+/// #pragma loop interleave(enable/disable/_value_)
+/// #pragma loop vectorize(...) interleave(...) ...
+/// \endcode
We normally express grammar fragments in comments in a BNF-like style.
Something like:
#pragma 'loop' loop-hints
loop-hints:
loop-hint loop-hints[opt]
loop-hint:
'vectorize' '(' loop-hint-value ')'
'interleave' '(' loop-hint-value ')'
loop-hint-value:
'enable'
'disable'
constant-expression
+/// Specifying vectorize(enable) or vectorize(_value_) instructs llvm to
+/// try vectorizing the subsequent loop. Specifying interleave(enable) or
+/// interleave(_value_) instructs llvm to tru interleaving the subsequent
loop
Typo 'tru'. Missing period.
+/// Giving a _value_ of 1 or "disable" prevents the optimization, even if
it
+/// is possible or profitable. The loop vectorizer currently only works on
+/// inner loops.
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?
+ PragmaLoopHintInfo *Info =
+ (PragmaLoopHintInfo*) PP.getPreprocessorAllocator().Allocate(
+ sizeof(PragmaLoopHintInfo), llvm::alignOf<PragmaLoopHintInfo>());
+
+ Info->Option = Tok;
+ StringRef Option = Tok.getIdentifierInfo()->getName();
+
+ if (Option != "vectorize" && Option != "interleave") {
+ PP.Diag(Tok.getLocation(), diag::warn_pragma_loop_invalid_option) <<
+ Option;
+ return;
+ }
Reorder this so that you don't perform the allocation if the option is
invalid.
+ // Read '('
+ PP.Lex(Tok);
+ if (Tok.isNot(tok::l_paren)) {
+ PP.Diag(Tok.getLocation(), diag::err_expected) << tok::l_paren;
+ return;
+ }
+
+ // Read the optimization value identifier.
+ PP.Lex(Tok);
+ if (Tok.isNot(tok::identifier) && Tok.isNot(tok::numeric_constant)) {
+ PP.Diag(Tok.getLocation(), diag::warn_pragma_loop_invalid_type) <<
+ Tok.getName();
+ return;
+ }
+
+ Info->Value = Tok;
+
+ // Read ')'
+ PP.Lex(Tok);
+ if (Tok.isNot(tok::r_paren)) {
+ PP.Diag(Tok.getLocation(), diag::err_expected) << tok::r_paren;
+ return;
+ }
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.
+ // Consume all remaining token in the pragam loop directive.
Typo 'pragam'.
+ PP.Diag(Tok.getLocation(), diag::warn_pragma_extra_tokens_at_eol) <<
(Here and elsewhere) << goes at the start of the line, not the end.
+ Token *TokenArray = new Token[TokenList.size()];
+ for(unsigned long i = 0; i < TokenList.size(); i++) {
+ TokenArray[i] = TokenList[i];
+ }
Use std::copy.
+ TokenList.clear();
Unnecessary; please remove.
+StmtResult Parser::ParsePragmaLoopHint(StmtVector &Stmts, bool
OnlyStatement,
+ SourceLocation *TrailingElseLoc,
+ ParsedAttributesWithRange &Attrs)
+{
+ // Create temporary attribute list.
+ ParsedAttributesWithRange TempAttrs(AttrFactory);
+
+ // Get vectorize hints and consume annotated token.
+ while (Tok.is(tok::annot_pragma_loop_hint)) {
+ LoopHint Hint = HandlePragmaLoopHint();
+ ConsumeToken();
+
+ if (!Hint.ValueLoc || !Hint.OptionLoc)
+ continue;
+
+ ArgsUnion ArgHints[] = {Hint.ValueLoc,
+ ArgsUnion(Hint.ValueExpr.get())};
+ TempAttrs.addNew(Hint.OptionLoc->Ident, Hint.Range,
+ 0, Hint.OptionLoc->Loc,
+ ArgHints, 2,
+ AttributeList::AS_Keyword);
+ }
+
+ // Get the next statement.
+ MaybeParseCXX11Attributes(Attrs);
+
+ if (isTokenSpecial()) {
+ Diag(Tok.getLocation(), diag::warn_pragma_loop_precedes_nonloop);
This special case makes no sense.
+ return StmtEmpty();
Why is this not an error?
+ }
+
+ Token StmtTok = Tok;
+ StmtResult S = ParseStatementOrDeclarationAfterAttributes(Stmts,
+ /*OnlyStatement*/ true, 0, Attrs);
+
+ // If it is a loop add the loop hint attributes to it.
+ if (StmtTok.is(tok::kw_for) || StmtTok.is(tok::kw_while) ||
StmtTok.is(tok::kw_do)) {
+ Attrs.takeAllFrom(TempAttrs);
+ return S;
+ }
+
+ Diag(StmtTok.getLocation(), diag::warn_pragma_loop_precedes_nonloop);
+ return S;
+}
Funneling your hints through the attribute mechanism seems very strange.
Instead, just parse a substatement then call a Sema function to
ActOnPragmaLoopHint(LoopHint, StmtResult).
On Tue, May 6, 2014 at 4:36 PM, Richard Smith <richard at metafoo.co.uk> 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 <nrotem at apple.com> 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 <aschwaighofer at apple.com>
>> 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 <tnowicki at apple.com> 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 <http://tok.is/>(tok::kw___attribute) &&
>> + (NextTok.is <http://nexttok.is/>(tok::kw_do) ||
>> + NextTok.is <http://nexttok.is/>(tok::kw_for) ||
>> + NextTok.is <http://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
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140506/1f1e9a25/attachment.html>
More information about the cfe-commits
mailing list