[PATCH] Pragma Loop constant expression support

[Richard Smith]

+++ b/lib/Parse/ParsePragma.cpp
[...]
+    if (R.isInvalid() ||
+      Actions.CheckLoopHintExpr(R.get(), Toks[0].getLocation()))

More indentation here please.

+  SmallVector<Token, 1> ValueList;
+  int OpenParens = ValueInParens ? 1 : 0;
+  // Read constant expression.
+  while (Tok.isNot(tok::eod)) {
+    if (Tok.is(tok::l_paren))
+      OpenParens++;
+ else if (Tok.is(tok::r_paren)) {
+      OpenParens--;
+  if(OpenParens == 0 && ValueInParens)
+  break;
     }

+ ValueList.push_back(Tok);
+ PP.Lex(Tok);
+  }

Some hard tab characters have snuck in here; please fix.

+  Token EoF;

OK, if we can't call this EOF then either EofTok or EOFTok? =)


+++ b/lib/Sema/SemaExpr.cpp
+  if (E->isValueDependent()) {
+    return false;
+  }

No braces here, please.

+  if (!ValueAPS.isStrictlyPositive() || ValueAPS.getBitWidth() > 32) {
+ Diag(E->getExprLoc(), diag::err_pragma_loop_invalid_argument_value) <<
ValueAPS.toString(10);
+    return true;
+  }

I don't see why we need to care about the width of the type here; it would
seem better to simply require that the value fits into 31 bits, if that's
the actual requirement imposed by the IR representation. That is, relpace
the getBitWidth() check with 'ValueAPS.getActiveBits() <= 31'. (As an
extreme example, consider a target where *all* integer types are 64 bits
wide.)

+++ b/test/CodeGen/pragma-loop.cpp
+// Test for scoped enumerations.
+enum Tuner { Interleave = 4,
+             Unroll = 8 };

This is an unscoped enumeration.

+++ b/include/clang/Basic/DiagnosticParseKinds.td
+def err_pragma_loop_missing_argument : Error<
+  "missing argument; expected %select{a positive 32-bit integer value|"

This "expected a positive 32-bit integer value" wording seems like the
wrong thing to be saying here, because it doesn't actually tell the user
what they missed out. It'd be much friendlier to say "expected loop unroll
count" or similar. Also, the sign and bit width information is a
distraction from the message.

+def err_pragma_loop_invalid_argument_type : Error<
+  "invalid argument %0; expected a 32-bit integer value">;

You should specifically say that the type is wrong here. Something like
"invalid argument of type %0; loop %select{unroll|interleave}1 count should
have integer type" would be better.

+def err_pragma_loop_invalid_argument_value : Error<
+  "invalid argument '%0'; expected a positive 32-bit integer value">;

Arguably 0x80000000u is a positive 32-bit integer value, but you reject it.
Something like "invalid argument %0; loop %select{unroll|interleave}1 count
must be %select{positive|%3 or smaller}2" would be nice.

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

This adds no information to the normal "not an ICE" diagnostic. Either
rewrite this to be friendlier ("loop unroll count is not an integral
constant expression") or just remove this and use the default diagnostic.


On Mon, Sep 15, 2014 at 1:12 PM, Tyler Nowicki <tyler.nowicki at gmail.com>
wrote:

> Ping... x2
>
> On Thu, Sep 11, 2014 at 2:38 PM, Tyler Nowicki <tyler.nowicki at gmail.com>
> wrote:
>
>> Ping...
>>
>> On Fri, Sep 5, 2014 at 1:30 PM, Tyler Nowicki <tyler.nowicki at gmail.com>
>> wrote:
>>
>>> Hi Richard,
>>>
>>> Thanks for the review. Sorry for the delayed response. I made the
>>> changes you suggested except for the following.
>>>
>>> +  Token EoF;
>>>> The 'o' in EOF is traditionally capitalized.
>>>
>>>
>>> Unfortunately EOF is defined by stdio.h and at least in VS the conflict
>>> triggers a compilation error. Also in PragmaMSPragma::HandlePragma EoF is
>>> used.
>>>
>>> Please review the attached patch.
>>>
>>> Thanks,
>>>
>>> Tyler
>>>
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140915/a5a45f4b/attachment.html>