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.



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

Unit TestsFailed

40 msx64 debian >
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/14.0.0/include -nostdsysteminc /var/lib/buildkite-agent/builds/llvm-project/clang/test/SemaOpenCL/ -verify -pedantic -fsyntax-only -cl-std=CL2.0

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

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


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

1 ↗(On Diff #380812)

I imagine we won't have a lot of functionality to test in this file and Then could we combine them in 1 file with a more generic name like or

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

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

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

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?


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?


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

Anastasia added inline comments.Dec 16 2021, 5:27 AM

We should be able to do this by using canonical type in the checks: