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

[Tyler Nowicki]

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 *.

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

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: pragma_nontypetemplate-svn.patch
Type: application/octet-stream
Size: 45516 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140708/998492bf/attachment.obj>