Differential D120254
[OpenCL] Align subgroup builtin guards Authored by svenvh on Feb 21 2022, 8:39 AM.
Details
Until now, subgroup builtins are available with opencl-c.h when at Align -fdeclare-opencl-builtins to opencl-c.h by introducing the
Diff Detail
Event Timeline
This comment was removed by • fonzvermund. This comment was removed by • fonzvermund. Comment Actions LGTM! Thanks!
Comment Actions Hi, our internal release build bots are showing failures in two clang-tidy tests that I bisected back to your commit, clang-tidy/checkers/altera-id-dependent-backward-branch.cpp and clang-tidy/checkers/altera-single-work-item-barrier.cpp. After this change, both are exhibiting this error: Error while processing /home/dyung/src/upstream/aa9c2d19d9b73589d72114d6e0a4fb4ce42b922b-linux/tools/clang/tools/extra/test/clang-tidy/checkers/Output/altera-single-work-item-barrier.cpp.tmp.cpp. error: enum type memory_scope not found; include the base header with -finclude-default-header [clang-diagnostic-error] Oddly, this only fails in a release configuration. Can you take a look? Comment Actions I'll try to reproduce the failure locally, but until I've done so perhaps you could try whether the following fixes one of the tests? If so, then the other test will likely need a similar fix. diff --git a/clang-tools-extra/test/clang-tidy/checkers/altera-id-dependent-backward-branch.cpp b/clang-tools-extra/test/clang-tidy/checkers/altera-id-dependent-backward-branch.cpp index a6dbab7b72fc..9bc1bbf173cc 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/altera-id-dependent-backward-branch.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/altera-id-dependent-backward-branch.cpp @@ -1,4 +1,4 @@ -// RUN: %check_clang_tidy %s altera-id-dependent-backward-branch %t -- -header-filter=.* "--" -cl-std=CL1.2 -c --include opencl-c.h +// RUN: %check_clang_tidy %s altera-id-dependent-backward-branch %t -- -header-filter=.* "--" -cl-std=CL1.2 -c --include opencl-c.h --include opencl-c-base.h typedef struct ExampleStruct { int IDDepField; Comment Actions If it helps, this is the cmake command I am using to build using gcc on linux: CMake command: -G Ninja -DLLVM_INCLUDE_EXAMPLES=OFF -DLLVM_VERSION_SUFFIX= -DLLVM_BUILD_RUNTIME=ON -DLLVM_TOOL_LLD_BUILD=OFF "-DLLVM_LIT_ARGS=--verbose -j80 --no-progress-bar --max-tests 10" -DCMAKE_BUILD_TYPE=Release -DLLVM_OPTIMIZED_TABLEGEN=ON -DLLVM_TARGETS_TO_BUILD=all -DLLVM_ENABLE_PROJECTS=clang;clang-tools-extra Some of the stuff there probably isn't needed (it's taken from a general script that I make small changes to when trying to reproduce stuff), but is there for completeness. Comment Actions Thanks, I could reproduce the problem with your cmake line. I have uploaded a fix for review in https://reviews.llvm.org/D120470 Comment Actions This was worked around by modifying tests, but I believe this is a fundamental problem in this change and was able to reproduce the error with plain old clang: $ cat test.cl void sub_group_barrier(); $ bin/clang -cl-std=CL1.2 -S -o - test.cl error: enum type memory_scope not found; include the base header with -finclude-default-header 1 error generated. $ bin/clang --version clang version 15.0.0 (git@github.com:llvm/llvm-project c204cee642ee794901d2e8a9819b52ac12f92bc9) Target: x86_64-unknown-linux-gnu Thread model: posix InstalledDir: /home/harald/llvm-project/build/bin The problem is that this change enables certain built-ins in OpenCL 1.2 that take a memory_scope argument, but the memory_scope type is not defined in OpenCL 1.2 mode. When we then process the function, sub_group_barrier in my example, things break when checking whether the declaration matches the built-in. I am not sure what the right fix here is. Can we just define the type if any extension is enabled that requires the type, or is that not allowed? Comment Actions Thanks for digging further and providing a reproducer! I think the fix is to only make the sub_group_barrier(cl_mem_fence_flags flags, memory_scope) overload available for OpenCL 2.0 or above. That would also match opencl-c.h. The following patch seems to fix the issue that you described: diff --git a/clang/lib/Sema/OpenCLBuiltins.td b/clang/lib/Sema/OpenCLBuiltins.td index f6de59223347..52740bacac33 100644 --- a/clang/lib/Sema/OpenCLBuiltins.td +++ b/clang/lib/Sema/OpenCLBuiltins.td @@ -1692,7 +1692,9 @@ let Extension = FuncExtKhrSubgroups in { // --- Table 28.2.2 --- let Extension = FuncExtKhrSubgroups in { def : Builtin<"sub_group_barrier", [Void, MemFenceFlags], Attr.Convergent>; - def : Builtin<"sub_group_barrier", [Void, MemFenceFlags, MemoryScope], Attr.Convergent>; + let MinVersion = CL20 in { + def : Builtin<"sub_group_barrier", [Void, MemFenceFlags, MemoryScope], Attr.Convergent>; + } } // --- Table 28.2.4 --- Comment Actions Huh, I could have sworn I saw more builtins than just this one but clearly I messed up there, that's the only one. That fix looks good to me, thanks, do you want to submit that as a new change along with the test or would you like a hand with it? Comment Actions I've submitted the fix in 4dfec37037f5. As for testing, unfortunately I couldn't just extend the SemaOpenCL/fdeclare-opencl-builtins.cl test: the OpenCL 1.2 RUN lines explicitly disable the cl_intel_subgroups extension (which is the extension that brings in sub_group_barrier with CL1.2 for the generic spir triple). Adding a new RUN line or new test file for just this case seems a bit of an overkill, so I'm tempted to defer this until we have parity between OpenCLBuiltins.td and opencl-c.h. | ||||||||||||||||||||
I'm in doubt whether we could just reuse __opencl_c_subgroups for this?