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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

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.

>> Note that this change also affects the code generation in one test:
>> because after 'const int n = 0' we now treat 'n' as a constant
>> expression with value 0, it's now a null pointer, so '(local int *)n'
>> forms a null pointer rather than a zero pointer.
>
> I am slightly confused about this case because technically the value of 'n' can be overwritten by casting away const.

That would result in undefined behavior. And if not, the same consideration would apply to array bounds :)

> However, I suspect this is compliant C99 behavior?

Not exactly, because C99 doesn't permit usage of variables in integer constant expressions; this is a consequence of the presumptive OpenCL rule that does permit such things. In C, the closest analogue would be something like:

  enum { e = 0 };
  int *p = (int*)0; // null pointer constant, not a bit-cast of zero to a pointer



> This example:
>
>   void test(void) {
>     const int x = 0;
>     const int* ptrx = &x;
>     int* ptry = (int*)ptrx;
>     *ptry = 100; 
>     int *sp5 = ( int*)x;
>   }
>
> shows that the IR produced is indeed supposed to have NULL despite of the fact that the value of initializing variable is not 0 at the time of the last initialization.

This example has undefined behavior due to writing to a const variable. Also, in C it initializes `sp5` to a zero pointer, not necessarily a null pointer, because `x` is not an integer constant expression -- though you can't see the difference here because we're in an address space where they're the same. Under the presumptive OpenCL rule, `x` would be an integer constant expression, so the above example would initialize `sp5` to a null pointer instead of a zero pointer, just like in C++98.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89520