Page MenuHomePhabricator

[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 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 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.