This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Add Sema checks for types
ClosedPublic

Authored by pxli168 on Feb 19 2016, 12:16 AM.

Diff Detail

Event Timeline

pxli168 updated this revision to Diff 48455.Feb 19 2016, 12:16 AM
pxli168 retitled this revision from to [OpenCL] Add Sema checks for image and pipe.
pxli168 updated this object.
pxli168 added a subscriber: cfe-commits.
yaxunl added inline comments.Feb 19 2016, 6:52 AM
test/SemaOpenCL/invalid-pipes-cl2.0.cl
9

Do we have a test for diagnosing pipe type for OCL < 2.0 ?

Anastasia added inline comments.Feb 19 2016, 11:41 AM
lib/Sema/SemaDecl.cpp
5714

Move to the line above, separate with -.

5725

Remove 'getLangOpts().OpenCLVersion >= 200'. Parser will only allow pipe for CL2.0. Ideally we don't need to check getLangOpts().OpenCL either, but might be good to leave it for documentation purposes.

However could we merge with the previous:

if (getLangOpts().OpenCL) {
 if (...) ... // image type
 if (...) ... // pipe type
}
10763

Here you only check the pointer and not the other bits. So please modify the comment according to what the code does.

Does the same restriction apply to other OpenCL types i.e. sampler, event, queue, etc?

test/CodeGenOpenCL/opencl_types.cl
39

Could you please not remove this mangling test as it's not being tested elsewhere.

You can remove * from parameters though!

pxli168 updated this revision to Diff 48654.Feb 22 2016, 12:46 AM
pxli168 retitled this revision from [OpenCL] Add Sema checks for image and pipe to [OpenCL] Add Sema checks for types.
pxli168 updated this object.
pxli168 marked an inline comment as done.

Refine comments and diag.

lib/Sema/SemaDecl.cpp
5725

Ok, it should be more clear.

10763

Good suggestion. I will have a look and try to add them.

test/CodeGenOpenCL/opencl_types.cl
39

OK, I will try to make this mangle right for different target.

test/SemaOpenCL/invalid-pipes-cl2.0.cl
9

We have one called test/SemaOpenCL/pipes-1.2-negative.cl

yaxunl edited edge metadata.Feb 22 2016, 7:52 AM

do we need to add a test that pointer to sampler is invalid?

Anastasia added inline comments.Feb 22 2016, 10:03 AM
test/SemaOpenCL/invalid-image.cl
3

Could we add similar tests for other types too. I think we could put them in existing cl files.

Anastasia added inline comments.Feb 22 2016, 10:07 AM
test/SemaOpenCL/invalid-image.cl
3

At least sampler test could go into test/SemaOpenCL/sampler_t.cl

pxli168 updated this revision to Diff 48773.Feb 22 2016, 8:56 PM
pxli168 edited edge metadata.

Add array check for types.

yaxunl accepted this revision.Feb 23 2016, 7:20 AM
yaxunl edited edge metadata.

LGTM. Thanks!

lib/Sema/SemaType.cpp
2180 ↗(On Diff #48773)

remove extra space

This revision is now accepted and ready to land.Feb 23 2016, 7:20 AM
Anastasia accepted this revision.Feb 23 2016, 8:49 AM
Anastasia edited edge metadata.
Anastasia added inline comments.
test/SemaOpenCL/sampler_t.cl
13 ↗(On Diff #48773)

put whitespace after ,

pxli168 updated this revision to Diff 48872.Feb 23 2016, 6:17 PM
pxli168 removed a reviewer: pekka.jaaskelainen.
pxli168 marked an inline comment as done.
pxli168 added a subscriber: pekka.jaaskelainen.
pxli168 closed this revision.Feb 24 2016, 7:38 PM