This is an archive of the discontinued LLVM Phabricator instance.

Don't permit array bound constant folding in OpenCL.
ClosedPublic

Authored by rsmith on Oct 15 2020, 5:56 PM.

Details

Summary

Permitting non-standards-driven "do the best you can" constant-folding
of array bounds is permitted solely as a GNU compatibility feature. We
should not be doing it in any language mode that is attempting to be
conforming.

From https://reviews.llvm.org/D20090 it appears the intent here was to
permit __constant int globals to be used in array bounds, but the
change in that patch only added half of the functionality necessary to
support that in the constant evaluator. This patch adds the other half
of the functionality and turns off constant folding for array bounds in
OpenCL.

I couldn't find any spec justification for accepting the kinds of cases
that D20090 accepts, so a reference to where in the OpenCL specification
this is permitted would be useful.

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.

Diff Detail

Event Timeline

rsmith created this revision.Oct 15 2020, 5:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 15 2020, 5:56 PM
rsmith requested review of this revision.Oct 15 2020, 5:56 PM

Permitting non-standards-driven "do the best you can" constant-folding
of array bounds is permitted solely as a GNU compatibility feature. We
should not be doing it in any language mode that is attempting to be
conforming.

From https://reviews.llvm.org/D20090 it appears the intent here was to
permit __constant int globals to be used in array bounds, but the
change in that patch only added half of the functionality necessary to
support that in the constant evaluator. This patch adds the other half
of the functionality and turns off constant folding for array bounds in
OpenCL.

I couldn't find any spec justification for accepting the kinds of cases
that 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++.

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. However, I suspect this is compliant C99 behavior?

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.

store i32* null, i32** %4, align 8

I couldn't find any spec justification for accepting the kinds of cases
that 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.

I am OK with the changes regarding null pointer. I guess people seldom set pointer to zero address in OpenCL.

I couldn't find any spec justification for accepting the kinds of cases
that 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.

Anastasia accepted this revision.Oct 19 2020, 3:02 AM

LGTM! Thanks

This revision is now accepted and ready to land.Oct 19 2020, 3:02 AM
This revision was landed with ongoing or failed builds.Oct 20 2020, 4:52 PM
This revision was automatically updated to reflect the committed changes.