[PATCH] #pragma vectorize

Tyler Nowicki tnowicki at apple.com
Wed May 7 14:14:28 PDT 2014


Hi Richard,

Thanks for the review. Here are my comments. An updated patch will follow due to reviews by Ben and Aaron.

> +  // 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<…>.

Sorry the comment was old, it does have a spelling that is used when creating the attr. The spelling indicates if it is a vectorize attribute or an interleave attribute. These could actually be separate attributes, although it doesn’t seem necessary because their behavior is the same. If a spelling is not specified the LoopHintAttr class is not generated by tablet gen.


> +    // 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.)

I will do this in a follow up commit. I experimented with it here, but found it might require a pre-defined class. Something like InheritableAttr.


> +    // 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.

I don’t follow your comments here. Could you elaborate?


> +  }
> +
> +  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).

What I really want is something analogous to LLVM IR metadata for stmts. However, this metadata has to be an AST node. AttributedStmts seem like the perfect way of passing information along with a stmt. Like an attribute this allows the loop hint to get to the code generator so metadata can be added to the conditional branch instruction on the loops. I also would like to add c++11 attributes that do the same thing as the #pragma loop directives. So it only seems natural to just turn #pragma loop into c++11 attributes.

I don’t understand how ‘ActOnPragmaLoopHint’ would be able to do that.

Tyler



More information about the cfe-commits mailing list