This is an archive of the discontinued LLVM Phabricator instance.

[OPENCL] Fix wrongly vla error for OpenCL array.
ClosedPublic

Authored by pxli168 on May 10 2016, 1:03 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

pxli168 updated this revision to Diff 56670.May 10 2016, 1:03 AM
pxli168 retitled this revision from to [OPENCL] Fix wrongly vla error for OpenCL array..
pxli168 updated this object.
pxli168 added reviewers: Anastasia, yaxunl.
pxli168 added subscribers: cfe-commits, bader.
yaxunl edited edge metadata.May 10 2016, 9:07 AM

LGTM. Thanks.

bader accepted this revision.May 10 2016, 9:12 AM
bader added a reviewer: bader.

LGTM

This revision is now accepted and ready to land.May 10 2016, 9:12 AM
Anastasia added inline comments.May 10 2016, 11:26 AM
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 added inline comments.May 10 2016, 7:08 PM
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 added inline comments.May 12 2016, 10:09 AM
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 added inline comments.May 26 2016, 7:41 PM
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 added inline comments.May 27 2016, 8:28 AM
test/CodeGenOpenCL/vla.cl
1 ↗(On Diff #56670)

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

pxli168 updated this revision to Diff 58932.May 29 2016, 10:42 PM
pxli168 edited edge metadata.

Make all tests in OpenCL 2.0.

Anastasia accepted this revision.Jun 3 2016, 9:39 AM
Anastasia edited edge metadata.

LGTM! Thanks!

This revision was automatically updated to reflect the committed changes.
rsmith added a subscriber: rsmith.Oct 15 2020, 1:27 PM
rsmith added inline comments.
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.