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

[Aaron Ballman]

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