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

[Richard Smith]

Thanks, this looks like the right approach.


+def err_pragma_loop_invalid_expression : Error<
+  "invalid argument; expected a constant integer expression">;

The appropriate term is "integer constant expression".


+    if (ValueExpr) {
+      llvm::APSInt ValueAPS;
+      bool IsICE = ValueExpr->isIntegerConstantExpr(ValueAPS,
CGM.getContext());
+      (void)IsICE; // So release build do not warn about unused variables
+      assert(IsICE &&
+             "Loop hint argument is not an integer constant expression");

Use 'ValueExpr->EvaluateKnownConstInt' here.


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


 struct PragmaLoopHintInfo {
   Token Loop;
-  Token Value;
   Token Option;
+  std::pair<Token *, size_t> Values;

Either use ArrayRef or split this out into two separate members with names
that indicate that one is the size of the other.


+  if (!Info)
+    return LoopHint(); // Uninitialized loop hints are ignored.

Why do we even create an annotation token in this case?


+    // Suppress diagnostic messages so we can report and invalid
expression if
+    // ParseConstantExpression fails.

Typo 'and'. But I don't think this is a good idea. If
ParseConstantExpression fails, you should just let it report its diagnostic
and bail out. It gives better diagnostics than you do.


+    PP.EnterTokenStream(Args, ArgSize, false, false);

You'll need to add a terminator token here to stop us falling off the end
of the tokens of the expression into whatever follows, in cases like

    #pragma clang loop vectorize_width(1 +)
    1
     for (...) {}

We usually add a tok::eof token after the saved tokens for this purpose.
Please also add /*comments*/ explaining what the 'false's mean in this call.


+    QualType QT = R.get()->getType();
+    if (!QT->isIntegerType() || QT->isBooleanType() || QT->isCharType()) {
+      Diag(Args[0].getLocation(),
diag::err_pragma_loop_invalid_expression);
+      return LoopHint(); // Uninitialized loop hints are ignored.
+    }

This should be in Sema, not here. (In particular, you need to allow a
dependent type here and repeat this check after instantiation.)


+    Token *TokenArray = new Token[ValueList.size()];

Allocate this on the Preprocessor's block allocator, instead of leaking it.


-    if (!Hint.LoopLoc || !Hint.OptionLoc || !Hint.ValueLoc)
+    if (!Hint.LoopLoc || !Hint.OptionLoc ||
+        !(Hint.EnabledLoc || Hint.ValueExpr))
       continue;

Maybe create a LoopHint::isInvalid() and put this there? Do you really need
to check the loop and option locations here?


+bool Sema::DiagnoseLoopHintValue(Expr *ValueExpr) {

Should be 'CheckLoopHintValue', since it doesn't always diagnose.


+  if (!ValueExpr->isIntegerConstantExpr(ValueAPS, getASTContext()) ||

Use VerifyIntegerConstantExpression here; it gives much better diagnostics.


+  // Only transform non-type template parameters.
+  if (!LH->getValue() || !LH->getValue()->isValueDependent())

This is not correct. You need to transform LH if the value is
instantiation-dependent too (also the comment is wrong...). The way we
usually deal with this is:

Expr *TransformedExpr = getDerived().TransformExpr(LH->getValue()).get();
if (TransformedExpr == LH->getValue().get())
  return LH;


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

Please add tests for the case where the value is type-dependent.

On Mon, Jun 30, 2014 at 3:07 PM, Hal Finkel <hfinkel at anl.gov> wrote:

> Tyler,
>
> Thanks for working on this!
>
> First, this patch contains a few things that looks like unrelated cleanup
> (formatting changes and extra CHECK lines in the tests). Please separate
> these out (and commit them separately).
>
> Except, not the formatting part of this change (because it makes the lines
> too long):
>
> -  /// State of the loop optimization specified by the spelling.
>
>    let Args = [EnumArgument<"Option", "OptionType",
>
> -                          ["vectorize", "vectorize_width", "interleave",
> "interleave_count",
>
> -                           "unroll", "unroll_count"],
>
> -                          ["Vectorize", "VectorizeWidth", "Interleave",
> "InterleaveCount",
>
> -                           "Unroll", "UnrollCount"]>,
>
> -              DefaultIntArgument<"Value", 1>];
>
> +                          ["vectorize", "vectorize_width", "interleave",
> "interleave_count", "unroll", "unroll_count"],
>
> +                          ["Vectorize", "VectorizeWidth", "Interleave",
> "InterleaveCount", "Unroll", "UnrollCount"]>,
>
> +              DefaultBoolArgument<"Enabled", 0>,
>
> +              ExprArgument<"Value", 0>];
>
> And, regarding errors like this:
> +/* expected-error {{invalid argument; expected a positive integer value}}
> */ #pragma clang loop interleave_count(10 / 5 - 2)
>
> I think it is important that we provide the evaluated integer value in the
> error message (it is obvious in this example, but if the numbers some from
> preprocessor macros and/or template parameters, it likely won't be
> obvious). Similarly, if we're given something of the wrong type (like a
> floating-point value), we should provide the name of the bad type.
>
> Thanks again,
> Hal
>
> ----- Original Message -----
> > From: "Tyler Nowicki" <tnowicki at apple.com>
> > To: "llvm cfe" <cfe-commits at cs.uiuc.edu>, "Hal Finkel" <hfinkel at anl.gov>,
> "Aaron Ballman" <aaron.ballman at gmail.com>,
> > "Richard Smith" <richard at metafoo.co.uk>
> > Cc: "Nadav Rotem" <nrotem at apple.com>
> > Sent: Friday, June 27, 2014 4:11:17 PM
> > Subject: [PATCH] Support constant expressions, including non-type
> template parameters, in pragma loop hints
> >
> > Hi Hal, Aaron, Richard,
> >
> > This patch adds support for constant expressions, including non-type
> > template parameters, in pragma loop hints.
> >
> > Support for constant expressions is done using
> > ParseConstantExpression() in the pragma parser, modifying the
> > LoopHintAttr to hold an Expr, and transforming the LoopHintAttr
> > expression during template instantiation.
> >
> > Previously it was suggested that I also split the LoopHintAttr into
> > an Attr for enable/disable loop hints and an Attr for loop hints
> > with an expression. I gave it a try but it caused a lot of
> > duplication without much of a reduction in the complexity of loop
> > hint handling. I am willing to give it another look if you think it
> > would be better in the long run.
> >
> > Tyler
> >
> >
> >
>
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140701/63cf7145/attachment.html>