[PATCH] Pragma Loop constant expression support

[Richard Smith]

On Wed, Sep 24, 2014 at 12:11 PM, Tyler Nowicki <tyler.nowicki at gmail.com>
wrote:

> Hi Richard,
>
> Thanks for the review. Sorry for the delayed response. I applied most of
> your suggestions. I have some comments on your suggestions for the
> diagnostic messages. Please review the attached patch.
>
>
>> +++ 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.
>>
>
> I went with "expected an integer value". The loop hint option, such as
> vectorize_width is already indicated by the error message with a caret
> pointing to it. Including more information makes the error message kind of
> redundant. Let me know what you think.
>

OK, I agree the meaning of the integer is sufficiently obvious from the
preceding identifier.


> +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.
>>
>
> I went with "expected an integer type". See my previous rational.
>
>
>
>> +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.
>>
>
> We cannot know what the maximums are at this point. Fortunately, the
> optimization passes will generate error messages when the loop directive
> cannot be followed. For example, an error is produce when the specified
> vector width is too large. So we just need to make sure the input fits into
> the IR. But we also shouldn't mislead the programmer with incorrect
> maximums. For example, 'loop unroll must be 1000000 or smaller' is a poor
> error because that maximum probably won't work.
>
> I went with 'invalid value '%0'; expected a positive 32-bit signed integer
> value'.
>

This still seems awkward to me. If you can't give an upper bound, then just
saying that it's too large would seem fine. 'invalid value %0; must be
positive' and 'value %0 is too large' or similar?

+  Token *TokenArray = (Token *)PP.getPreprocessorAllocator().Allocate(
+      ValueList.size() * sizeof(Token), llvm::alignOf<Token>());

Can you use

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

here?

Anyway... LGTM. I'm fine with improving the "positive 32-bit signed integer
value" wording post-commit if you'd prefer.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140929/10fe1b66/attachment.html>