On Wed, May 7, 2014 at 5:14 PM, Tyler Nowicki <[email protected]> wrote:
> 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.
It doesn't really require a spelling because it's not something that
the user can type out directly. If you went with two attributes
instead of one, then you don't require a spelling for it. You could
get away with something like:
def LoopHint : Attr {
let Args = [IntArgument<"type">,
ExprArgument<"value", 1>];
let AdditionalMembers = [{
// Your static methods
}];
}
def VectorizeLoopHint : LoopHint;
def InterleaveLoopHint : LoopHint;
Pretty sure that would work for you. You definitely do not need a
spelling to get the class generated by tablegen (there are several
other attributes which are implicit-only and have no spelling).
>
>
>> + // 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.
I have a weak preference for this to be done with this patch, instead
of a follow-up one. But it's only a weak preference.
>
>
>> + // 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
~Aaron
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
