[PATCH] Support constant expressions, including non-type template parameters, in pragma loop hints

[Aaron Ballman]

On Thu, Jul 24, 2014 at 6:48 PM, Tyler Nowicki <tnowicki at apple.com> wrote:
> Hi Aaron,
>
> Thanks for the review!
>
> There were a number of conflicts with the work on pragma unroll and nounroll. To accommodate these directives I changed the Enabled argument of the loop hint attribute to a state argument. The state argument can be default, enable, or disable. Attributes like ‘#pragma unroll’ that don’t have any arguments use the default state.

I would prefer this to be a separate change if at all possible (as a
predecessor to this patch) as it seems to be fairly substantive --
there's about a 17kb difference between this patch and the last one.
;-)

>
> I also applied most of your suggestions except for those below. Please review the updated patch.
>
>
>>> +    ConsumeToken(); // The annotation token.
>>
>> I would hoist this out of the if statement and place it closer to the
>> assert at the top of the method.
> (see next)
>
>>> +    // Enter constant expression including eof terminator into token stream.
>>> +    PP.EnterTokenStream(Args, ArgSize, /*DisableMacroExpansion=*/false,
>>> +                        /*OwnsTokens=*/false);
>>> +
>>> +    ConsumeToken(); // The annotation token.
>>
>> This winds up being hoisted and so can be removed.
>
> Moving a PP.Lex before the PP.EnterTokenStream means that when ParseConstantExpression() is called Tok is set to the token that follows the annotation token, but thats not what we want. What want Tok to be the first token in the constant expression. In other words
>
> ConsumeToken
> PP.EnterTokenStream
>
> isn’t the same as
>
> PP.EnterTokenStream
> ConsumeToken
>
> So we can’t hoist ConsumeToken to the top of the function.

Ahh, okay, that makes sense. Thank you for the explanation!

>
>
>>> +
>>> +    ExprResult R = ParseConstantExpression();
>>> +
>>> +    ConsumeToken(); // The constant expression eof terminator.
>>
>> You should not be assuming this is an eof terminator -- since this is
>> parser functionality, I think using ExpectAndConsume would make the
>> most sense (so you can diagnose if the token isn't present, and fail
>> out properly).
>>
>> A parsing tese where this fails would be good as well.
>
> The terminator is added after the pragma is parsed. I’m not sure how to write a test that makes ParseConstantExpression() fail to stop at the terminator. The terminator (eof) is specifically added to make sure ParseConstantExpression() doesn’t parse past the end of the directive.

Then I would add an assert ensuring that you really have consumed the
EOF and not something else.

> What can go wrong here is ParseConstantExpression() can stop before it has consumed all of the tokens in the constant expression. Because the expression was ill-formed something like '(1 2)’. It will parse ‘1’ and leave ‘2’ on the token stream. The pragma unroll test ‘#pragma unroll 1 2’ tests exactly this. I modified the code here to emit a warning and consume all remaining tokens in the constant expression.

Ah, great!

>
>
>>> void EmitClangAttrList(RecordKeeper &Records, raw_ostream &OS) {
>>>   emitSourceFileHeader("List of all attributes that Clang recognizes", OS);
>>> @@ -1633,14 +1641,25 @@
>>>         " INHERITABLE_PARAM_ATTR(NAME)\n";
>>>   OS << "#endif\n\n";
>>>
>>> +  OS << "#ifndef PRAGMA_SPELLING_ATTR\n";
>>> +  OS << "#define PRAGMA_SPELLING_ATTR(NAME)\n";
>>> +  OS << "#endif\n\n";
>>> +
>>> +  OS << "#ifndef LAST_PRAGMA_SPELLING_ATTR\n";
>>> +  OS << "#define LAST_PRAGMA_SPELLING_ATTR(NAME) PRAGMA_SPELLING_ATTR(NAME)\n";
>>> +  OS << "#endif\n\n";
>>> +
>>>   Record *InhClass = Records.getClass("InheritableAttr");
>>>   Record *InhParamClass = Records.getClass("InheritableParamAttr");
>>> -  std::vector<Record*> Attrs = Records.getAllDerivedDefinitions("Attr"),
>>> -                       NonInhAttrs, InhAttrs, InhParamAttrs;
>>> +  std::vector<Record *> Attrs = Records.getAllDerivedDefinitions("Attr"),
>>> +                        NonInhAttrs, InhAttrs, InhParamAttrs, PragmaAttrs;
>>>   for (auto *Attr : Attrs) {
>>>     if (!Attr->getValueAsBit("ASTNode"))
>>>       continue;
>>> -
>>> +
>>> +    if (AttrHasPragmaSpelling(Attr))
>>> +      PragmaAttrs.push_back(Attr);
>>> +
>>
>> I think this is fine for now, but I am starting to get worried about
>> the maintainability of this list. It used to be about type
>> categorization, now it's a bit larger. But then again, the type
>> categorization wasn't always perfect either (for instance, something
>> can be a type attribute and an inheritable attribute at the same
>> time); we just sort of skirted around it.
>>
>> So at some point, this code may need to be revisited, but it seems
>> correct right now.
>
> Ok, thats great. The new list is used by sema during template instantiation. Rather than hard-coding the attributes that require a special template instantiation this list is used to automatically generate transform function stubs and their uses. Doing this for every attribute, not just attributes with pragma spellings, would be bad because most attributes can only be applied to declarations which are handled in a completely different way. So it would create many useless stub functions.

Good to know.

~Aaron