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

[Tyler Nowicki]

Sorry I generated the patch incorrectly. Here is the correct patch.

I also fixed the parsing off-the-end bug using the EoF terminator that was suggestsd before and added test for it.

Let me know what you think,

Tyler


On Jul 9, 2014, at 7:00 AM, Aaron Ballman <aaron.ballman at gmail.com> wrote:

> On Tue, Jul 8, 2014 at 10:41 PM, Tyler Nowicki <tnowicki at apple.com> wrote:
>> Hi,
>> 
>> Thanks for all the feedback. I’ve applied your suggestions. Please see the attached patch.
>> 
>> I tried terminating the token list with EoF but it didn’t produce the desired result. I don’t know enough about parsing to even try to say what was happening. It seemed like something about the parsers state was messed up after the eof. Also I tried the example, vectorize_width(1+) 1 and it does seem to consume tokens past the end of the directive. Any ideas how I can fix this?
>> 
>>>> +bool Sema::DiagnoseLoopHintValue(Expr *ValueExpr) {
>>> 
>>> This should take a const Expr *.
>> 
>> 
>> Unfortunately VerifyIntegerConstantExpression() for getting the IntAPS of the expression doesn’t take a const Expr *, although I don’t think it modifies the result. So we have to pass in an Expr *.
> 
> Ah, yes, and changing VerifyIntegerConstantExpression() would require
> updating quite a few signatures. That makes sense.
> 
>> 
>>>> +    if (!R.isUsable()) {
>>>> +      Diag(Args[0].getLocation(), diag::err_pragma_loop_invalid_expression);
>>>> +      return LoopHint(); // Uninitialized loop hints are ignored.
>>>> +    }
>>>> +
>>>> +    QualType QT = R.get()->getType();
>>>> +    if (!QT->isIntegerType() || QT->isBooleanType() || QT->isCharType()) {
>>> 
>>> Are char and bool types truly that heinous? (I agree they would look
>>> odd, but do they require restricting?). What about scoped
>>> enumerations?
>> 
>> I don’t like the idea of allowing statements like #pragma clang loop vectorize_width(‘A’) because it is unclear what we should do. What is an ‘A’ width? I’ve added a test for scoped enumerations.
> 
> Fair.
> 
>> 
>>> +  /// \brief Transform the given attribute.
>>> +  ///
>>> +  /// By default, this routine transforms a statement by delegating to the
>>> +  /// appropriate TransformXXXAttr function to transform a specific kind
>>> +  /// of attribute. Subclasses may override this function to transform
>>> +  /// attributed statements using some other mechanism.
>>> +  ///
>>> +  /// \returns the transformed attribute
>>> +  const Attr *TransformAttr(const Attr *S);
>>> 
>>> I don't like that this gives us two completely different ways to instantiate attributes (TreeTransform::transformAttrs / Sema::InstantiateAttrs and this new function). I'm OK with this as a stopgap measure if you're prepared to clean this duplication up afterwards, but this is not reasonable as a long-term situation.
>> 
>> Each is used for a different kind of attribute. Attributes on declaration and attributes on statements. Each attribute is handled in a completely different manner throughout clang. For example, statement attributes are stored in a wrapper called an AttributedStmt while declaration attributes are stored in a list that is mapped to the pointer of the Decl object.  I’m not sure why they are treated differently, but it would be a lot of work to unify them. There are currently only two statement attributes: loop hint and switch fallthrough.
>> 
>>> +      ValueInt = ValueAPS.getSExtValue();
>>> +    }
>>> 
>>> What if the value doesn't fit in 64 bits? Asserting is not a friendly response. Since you're looking for a 32-bit int anyway, maybe saturate at 0xFFFFF’FFFF?
>> 
>> Not sure what you mean by saturate at 0xFF…? The loop hint metadata only takes a 32-bit value so we can’t support anything more than that. I added a condition on the value that it be represented with at most 32 bits. Otherwise an invalid_argument error will be triggered which uses a special print method to report the erroneous value. There is already a test for it.
>> 
>> Tyler
>> 
> 
> Btw, I am unable to apply your patch to ToT with svn. I get very odd
> results. Eg) Sema.h
> 
> bool CheckLoopHintExpr(Expr *E, SourceLocation Loc, bool AllowValueDependent);
> 
>                    ArrayRef<Expr *> Args,
>                    SourceLocation LitEndLoc,
>                    TemplateArgumentListInfo *ExplicitTemplateArgs = nullptr);
> 
> ~Aaron

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140709/c5d5b05c/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pragma_nontypetemplate-svn.patch
Type: application/octet-stream
Size: 46085 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140709/c5d5b05c/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140709/c5d5b05c/attachment-0001.html>