[OPENCL] Fix wrongly vla error for OpenCL array.

[pxli168]

Summary

OpenCL should support array with const value size length, those const varibale in global and constant address space and variable in constant address space.

[yaxunl]

LGTM. Thanks.

[bader]

LGTM

[Anastasia]

lib/Sema/SemaType.cpp

2055 ↗

(On Diff #56670)

Formatting looks weird though... may be better to leave the first line unmodified?

test/CodeGenOpenCL/vla.cl

1 ↗

(On Diff #56670)

Could we have a Sema test instead where we accept the VLA if constant AS object is used and give an error otherwise?

Also do we need to test CL2.0? We don't seem to be doing anything different for that version.

[pxli168]

lib/Sema/SemaType.cpp

2055 ↗

(On Diff #56670)

I felt very strange, too. But this is what clang-format gives me.

test/CodeGenOpenCL/vla.cl

1 ↗

(On Diff #56670)

In earier than OpenCL1.2 program scope variable must reside in constant address space and no

const global int

can be here.
But const global value should also not be seen as VLA either.

[Anastasia]

lib/Sema/SemaType.cpp

2055 ↗

(On Diff #56670)

I see. clang-format doesn't take readability into account unfortunately. :)

Let's just leave the first line of this change unmodified at least to make it more readable.

test/CodeGenOpenCL/vla.cl

1 ↗

(On Diff #56670)

Why not? Could you write a complete example perhaps? I am not sure I understand your point.

Thanks!

[pxli168]

lib/Sema/SemaType.cpp

2055 ↗

(On Diff #56670)

OK, I also found the clang-format can do some strange thing.

test/CodeGenOpenCL/vla.cl

1 ↗

(On Diff #56670)

Oh, I mean that in OpenCL 1.2 there will be some error message with

const global int

But I want to have a test with something that not only with the constant address space.
Or what you suggest is that I make all the test in OpenCL 2.0?

[Anastasia]

test/CodeGenOpenCL/vla.cl

1 ↗

(On Diff #56670)

Yes, I think this should be enough for testing your change.

[pxli168]

Make all tests in OpenCL 2.0.

[Anastasia]

LGTM! Thanks!

[rsmith]

cfe/trunk/lib/Sema/SemaType.cpp

2067

This looks wrong to me. The OpenCL rules don't permit arbitrary constant folding in array bounds.

If OpenCL intends to permit reading from const globals of integral types in constant expressions (as C++ does but C does not), then the right way to handle that would be to change CheckICE to permit such cases, as it does in C++ mode, not to enable arbitrary constant folding in array bounds.

cfe/trunk/lib/AST/ExprConstant.cpp

if (!Frame) {
         // evaluation.
       } else if (AK != AK_Read) {
         // All the remaining cases only permit reading.
         Info.Diag(E, diag::note_constexpr_modify_global);
         return CompleteObject();
       } else if (VD->isConstexpr()) {
         // OK, we can read this variable.
       } else if (BaseType->isIntegralOrEnumerationType()) {
-        if (!BaseType.isConstQualified()) {
+        // In OpenCL if a variable is in constant address space it is a const value.
+        if (!(BaseType.isConstQualified() ||
+              (Info.getLangOpts().OpenCL &&
+               BaseType.getAddressSpace() == LangAS::opencl_constant))) {
           if (Info.getLangOpts().CPlusPlus) {
             Info.Diag(E, diag::note_constexpr_ltor_non_const_int, 1) << VD;
             Info.Note(VD->getLocation(), diag::note_declared_at);
           } else {
             Info.Diag(E);
           }
           return CompleteObject();
         }

cfe/trunk/lib/Sema/SemaType.cpp

public:
     }
 
     void diagnoseFold(Sema &S, SourceLocation Loc, SourceRange SR) override {
       S.Diag(Loc, diag::ext_vla_folded_to_constant) << SR;
     }
   } Diagnoser;
 
   return S.VerifyIntegerConstantExpression(ArraySize, &SizeVal, Diagnoser,
-                                           S.LangOpts.GNUMode).isInvalid();
+                                           S.LangOpts.GNUMode ||
+                                           S.LangOpts.OpenCL).isInvalid();
 }
 
 /// \brief Build an array type.
 ///
 /// \param T The type of each element in the array.
 ///
 /// \param ASM C99 array size modifier (e.g., '*', 'static').
 ///

cfe/trunk/test/CodeGenOpenCL/vla.cl

+// RUN: %clang_cc1 -emit-llvm -O0 -cl-std=CL2.0 -o - %s | FileCheck %s
+
+constant int sz0 = 5;
+// CHECK: @sz0 = constant i32 5, align 4
+const global int sz1 = 16;
+// CHECK: @sz1 = constant i32 16, align 4
+const constant int sz2 = 8;
+// CHECK: @sz2 = constant i32 8, align 4
+// CHECK: @testvla.vla2 = internal global [8 x i16] undef, align 16
+
+kernel void testvla()
+{
+  int vla0[sz0];
+// CHECK: %vla0 = alloca [5 x i32], align 16
+  char vla1[sz1];
+// CHECK: %vla1 = alloca [16 x i8], align 16
+  local short vla2[sz2];
+}