[PATCH] D89520: Don't permit array bound constant folding in OpenCL.

Mon Oct 19 02:57:53 PDT 2020

Anastasia added a comment.

In D89520#2335628 <https://reviews.llvm.org/D89520#2335628>, @rsmith wrote:

> In D89520#2334477 <https://reviews.llvm.org/D89520#2334477>, @Anastasia wrote:
>
>>> I couldn't find any spec justification for accepting the kinds of cases
>>> that D20090 <https://reviews.llvm.org/D20090> accepts, so a reference to where in the OpenCL specification
>>> this is permitted would be useful.
>>
>> Thanks for fixing this. I agree that the original change was not compliant with the spec. OpenCL indeed doesn't allow constant folding for array bounds. The idea of the change was to allow using expressions that are compile time constant in the array bound because this doesn't result in VLA.
>>
>> Regarding the spec reference, I think we can refer to the section 6.5.3 describing variables in the `__constant` address space:
>>
>>   These variables  are  required  to  be  initialized  and  the  values  used  to  initialize  these  variables  must  be  a compile time constant. Writing to such a variable results in a compile-time error.
>>
>> I.e. the `__constant` address space variables are semantically similar to `constexpr` in C++.
>
> I don't see any way to read this wording that way. That talks about how the variables are initialized and says they're immutable, but I can't find anything in the OpenCL specification that says they can be used in integer constant expressions. The C definition of "integral constant expression" does not allow the use of variables:
>
> C11 6.6/6: "An integer constant expression shall have integer type and shall only have operands that are integer constants, enumeration constants, character constants, sizeof expressions whose results are integer constants, _Alignof expressions, and floating constants that are the immediate operands of casts."
>
> ... and absent a modification to that, nor would OpenCL. That said, I'm happy to take your word for it that the OpenCL specification //intends// for such variable uses to be permitted in constant expressions.

Yes, I agree it is not explicit. I will attempt to improve it. However considering that your change doesn't add this feature but modifies existing implementation, I think it makes sense to go ahead.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89520/new/

https://reviews.llvm.org/D89520