Page MenuHomePhabricator

[OpenCL] queue_t and ndrange_t can't be defined in program scope.
Needs ReviewPublic

Authored by cycheng on Oct 19 2021, 4:27 PM.

Details

Reviewers
Anastasia
Summary

OpenCL v2.0 s6.5.1 and OpenCL v3.0 s6.11.w both state: Program scope variables can be defined with any valid OpenCL C data type except for those in Other Built-in Data Types.

This patch adds the required check in diagnoseOpenCLTypes.

Diff Detail

Event Timeline

cycheng created this revision.Oct 19 2021, 4:27 PM
cycheng requested review of this revision.Oct 19 2021, 4:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 19 2021, 4:27 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Anastasia added inline comments.Oct 26 2021, 12:39 PM
clang/lib/Sema/SemaDecl.cpp
6844

To make things concise I would suggest dropping OpenCL v2.0 quote since it is identical to OpenCL v3.0.

6853

Suggest swapping those as string comparisons are more costly.

However, does it work if there is a typedef of ndrange_t to some other name? Although it feels like the same issue might apply to other types diagnosed here...

clang/test/SemaOpenCL/invalid-ndrange.cl
1

I imagine we won't have a lot of functionality to test in this file and invalid-queue.cl? Then could we combine them in 1 file with a more generic name like invalid_types.cl or invalid_enqueue_types.cl.

This can spare us extra clang invocations...

cycheng updated this revision to Diff 382813.Oct 27 2021, 3:15 PM
cycheng marked 2 inline comments as done.
  • Remove OpenCL 2.0 description
  • Merge test files into single test file
cycheng updated this revision to Diff 382816.Oct 27 2021, 3:19 PM

previous update was using wrong file, sorry!!

Anastasia added inline comments.Nov 8 2021, 4:06 AM
clang/test/SemaOpenCL/invalid-types.cl
5 ↗(On Diff #382816)

if you create a type alias through typedef for queue_t or ndrange_t, will you still get the diagnostic?

Anastasia added inline comments.Dec 2 2021, 5:00 AM
clang/test/SemaOpenCL/invalid-types.cl
10 ↗(On Diff #382816)

Did you mean to use ndrange_t or queue_t here? Otherwise, it doesn't seem that your commit is related to the image types...

cycheng added inline comments.Dec 2 2021, 1:12 PM
clang/lib/Sema/SemaDecl.cpp
6853

does it work if there is a typedef of ndrange_t to some other name?

No, should we recursively check it?
Or do we have better way to handle this?

clang/test/SemaOpenCL/invalid-types.cl
5 ↗(On Diff #382816)

Sorry for late reply!
No, I didn't catch that case, I am not sure how to handle this? Could you point out some code for reference?

10 ↗(On Diff #382816)

Ah... sorry it's an accident. I will remove it.

Anastasia added inline comments.Dec 16 2021, 5:27 AM
clang/lib/Sema/SemaDecl.cpp
6853

We should be able to do this by using canonical type in the checks:
https://clang.llvm.org/doxygen/classclang_1_1QualType.html#a900520b154ba282febbaa7267020ca3f