[Offload][SYCL] Refactor OffloadKind implementation #135809

asudarsa · 2025-04-15T16:30:31Z

Following are the changes:

Make OffloadKind enum values to be powers of two so we can use them like a bitfield
Include OFK_SYCL enum value
Modify ActiveOffloadKinds support in clang-linker-wrapper to use bitfields instead of a vector.

Thanks

Signed-off-by: Arvind Sudarsanam <[email protected]>

llvmbot · 2025-04-15T16:31:07Z

@llvm/pr-subscribers-clang-driver
@llvm/pr-subscribers-llvm-binary-utilities

@llvm/pr-subscribers-clang

Author: Arvind Sudarsanam (asudarsa)

Changes

Following are the changes:

Make OffloadKind enum values to be powers of two so we can use them like a bitfield
Include OFK_SYCL enum value
Modify ActiveOffloadKinds support in clang-linker-wrapper to use bitfields instead of a vector.

Thanks

Full diff: https://github.com/llvm/llvm-project/pull/135809.diff

2 Files Affected:

(modified) clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp (+6-4)
(modified) llvm/include/llvm/Object/OffloadBinary.h (+6-4)

diff --git a/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp b/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
index 52d922abbcaec..a76b6f1da1e1d 100644
--- a/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
+++ b/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
@@ -923,10 +923,9 @@ Expected<SmallVector<StringRef>> linkAndWrapDeviceFiles(
         });
     auto LinkerArgs = getLinkerArgs(Input, BaseArgs);
 
-    DenseSet<OffloadKind> ActiveOffloadKinds;
+    uint16_t ActiveOffloadKindMask = 0u;
     for (const auto &File : Input)
-      if (File.getBinary()->getOffloadKind() != OFK_None)
-        ActiveOffloadKinds.insert(File.getBinary()->getOffloadKind());
+      ActiveOffloadKindMask |= File.getBinary()->getOffloadKind();
 
     // Write any remaining device inputs to an output file.
     SmallVector<StringRef> InputFiles;
@@ -943,7 +942,10 @@ Expected<SmallVector<StringRef>> linkAndWrapDeviceFiles(
       return OutputOrErr.takeError();
 
     // Store the offloading image for each linked output file.
-    for (OffloadKind Kind : ActiveOffloadKinds) {
+    for (OffloadKind Kind = OFK_FIRST; Kind != OFK_LAST;
+         Kind = static_cast<OffloadKind>((uint16_t)(Kind) << 1)) {
+      if ((ActiveOffloadKindMask & Kind) == 0)
+        continue;
       llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> FileOrErr =
           llvm::MemoryBuffer::getFileOrSTDIN(*OutputOrErr);
       if (std::error_code EC = FileOrErr.getError()) {
diff --git a/llvm/include/llvm/Object/OffloadBinary.h b/llvm/include/llvm/Object/OffloadBinary.h
index c02aec8d956ed..858c7a59bce4a 100644
--- a/llvm/include/llvm/Object/OffloadBinary.h
+++ b/llvm/include/llvm/Object/OffloadBinary.h
@@ -32,10 +32,12 @@ namespace object {
 /// The producer of the associated offloading image.
 enum OffloadKind : uint16_t {
   OFK_None = 0,
-  OFK_OpenMP,
-  OFK_Cuda,
-  OFK_HIP,
-  OFK_LAST,
+  OFK_OpenMP = (1 << 1),
+  OFK_FIRST = OFK_OpenMP,
+  OFK_Cuda = (1 << 2),
+  OFK_HIP = (1 << 3),
+  OFK_SYCL = (1 << 4),
+  OFK_LAST = (1 << 5),
 };
 
 /// The type of contents the offloading image contains.

jhuber6 · 2025-04-15T16:32:46Z

llvm/include/llvm/Object/OffloadBinary.h

-  OFK_HIP,
-  OFK_LAST,
+  OFK_OpenMP = (1 << 1),
+  OFK_FIRST = OFK_OpenMP,


We don't need first.

Good catch. I will update.

Thanks

Addressed in c0bc689

Thanks

jhuber6 · 2025-04-15T16:33:03Z

llvm/include/llvm/Object/OffloadBinary.h

-  OFK_Cuda,
-  OFK_HIP,
-  OFK_LAST,
+  OFK_OpenMP = (1 << 1),


This is 2, not 1.

Addressed in c0bc689

Thanks

jhuber6 · 2025-04-15T16:33:29Z

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp

@@ -923,10 +923,9 @@ Expected<SmallVector<StringRef>> linkAndWrapDeviceFiles(
        });
    auto LinkerArgs = getLinkerArgs(Input, BaseArgs);

-    DenseSet<OffloadKind> ActiveOffloadKinds;
+    uint16_t ActiveOffloadKindMask = 0u;


This code doesn't need to be modified, but I guess we could if we wanted to get rid of the set. I don't think it's necessary now though.

I think it is a good optimization to have (16-bit mask instead of a Set). Also, it will be easier to pass the mask to callee function when introducing support for SYCL in my original PR.

Thanks

Signed-off-by: Arvind Sudarsanam <[email protected]>

…P from 3 to 4 Signed-off-by: Arvind Sudarsanam <[email protected]>

llvm-ci · 2025-04-16T14:32:13Z

LLVM Buildbot has detected a new failure on builder llvm-clang-x86_64-gcc-ubuntu running on sie-linux-worker3 while building clang,llvm at step 6 "test-build-unified-tree-check-all".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/174/builds/16286

Here is the relevant piece of the build log for the reference

Step 6 (test-build-unified-tree-check-all) failure: test (failure)
******************** TEST 'AddressSanitizer-x86_64-linux :: TestCases/asan_lsan_deadlock.cpp' FAILED ********************
Exit Code: 1

Command Output (stderr):
--
/home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/build/./bin/clang  --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only  -m64  -O0 /home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/llvm-project/compiler-rt/test/asan/TestCases/asan_lsan_deadlock.cpp -o /home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/build/runtimes/runtimes-bins/compiler-rt/test/asan/X86_64LinuxConfig/TestCases/Output/asan_lsan_deadlock.cpp.tmp # RUN: at line 4
+ /home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/build/./bin/clang --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -m64 -O0 /home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/llvm-project/compiler-rt/test/asan/TestCases/asan_lsan_deadlock.cpp -o /home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/build/runtimes/runtimes-bins/compiler-rt/test/asan/X86_64LinuxConfig/TestCases/Output/asan_lsan_deadlock.cpp.tmp
env ASAN_OPTIONS=detect_leaks=1 not  /home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/build/runtimes/runtimes-bins/compiler-rt/test/asan/X86_64LinuxConfig/TestCases/Output/asan_lsan_deadlock.cpp.tmp 2>&1 | FileCheck /home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/llvm-project/compiler-rt/test/asan/TestCases/asan_lsan_deadlock.cpp # RUN: at line 5
+ env ASAN_OPTIONS=detect_leaks=1 not /home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/build/runtimes/runtimes-bins/compiler-rt/test/asan/X86_64LinuxConfig/TestCases/Output/asan_lsan_deadlock.cpp.tmp
+ FileCheck /home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/llvm-project/compiler-rt/test/asan/TestCases/asan_lsan_deadlock.cpp
�[1m/home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/llvm-project/compiler-rt/test/asan/TestCases/asan_lsan_deadlock.cpp:58:12: �[0m�[0;1;31merror: �[0m�[1mCHECK: expected string not found in input
�[0m // CHECK: SUMMARY: AddressSanitizer: stack-buffer-overflow
�[0;1;32m           ^
�[0m�[1m<stdin>:1:1: �[0m�[0;1;30mnote: �[0m�[1mscanning from here
�[0m=================================================================
�[0;1;32m^
�[0m�[1m<stdin>:2:10: �[0m�[0;1;30mnote: �[0m�[1mpossible intended match here
�[0m==1774144==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7b8e9eade034 at pc 0x5617cd679260 bp 0x7b8e9ccfdce0 sp 0x7b8e9ccfdcd8
�[0;1;32m         ^
�[0m
Input file: <stdin>
Check file: /home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/llvm-project/compiler-rt/test/asan/TestCases/asan_lsan_deadlock.cpp

-dump-input=help explains the following input dump.

Input was:
<<<<<<
�[1m�[0m�[0;1;30m            1: �[0m�[1m�[0;1;46m================================================================= �[0m
�[0;1;31mcheck:58'0     X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
�[0m�[0;1;30m            2: �[0m�[1m�[0;1;46m==1774144==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7b8e9eade034 at pc 0x5617cd679260 bp 0x7b8e9ccfdce0 sp 0x7b8e9ccfdcd8 �[0m
�[0;1;31mcheck:58'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
�[0m�[0;1;35mcheck:58'1              ?                                                                                                                                    possible intended match
�[0m�[0;1;30m            3: �[0m�[1m�[0;1;46mWRITE of size 4 at 0x7b8e9eade034 thread T2 �[0m
�[0;1;31mcheck:58'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
�[0m�[0;1;30m            4: �[0m�[1m�[0;1;46mTimeout! Deadlock detected. �[0m
�[0;1;31mcheck:58'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
�[0m>>>>>>

--

********************

asudarsa · 2025-04-16T15:00:42Z

AddressSanitizer test is passing in subsequent runs. Mostly a flaky issue.

Thanks

Following are the changes: 1. Make OffloadKind enum values to be powers of two so we can use them like a bitfield 2. Include OFK_SYCL enum value 3. Modify ActiveOffloadKinds support in clang-linker-wrapper to use bitfields instead of a vector. Thanks --------- Signed-off-by: Arvind Sudarsanam <[email protected]>

[Offload][SYCL] Refactor OffloadKind implementation

ec072a0

Signed-off-by: Arvind Sudarsanam <[email protected]>

llvmbot added clang Clang issues not falling into any other category llvm:binary-utilities labels Apr 15, 2025

asudarsa mentioned this pull request Apr 15, 2025

[SYCL] Add clang-linker-wrapper changes to call clang-sycl-linker for SYCL offloads #135683

Merged

jhuber6 reviewed Apr 15, 2025

View reviewed changes

Address review comments

c0bc689

Signed-off-by: Arvind Sudarsanam <[email protected]>

asudarsa requested a review from jhuber6 April 15, 2025 17:46

jhuber6 approved these changes Apr 15, 2025

View reviewed changes

Update tests to reflect changes in Offload kind enum value for OFK_HI…

ed2b73c

…P from 3 to 4 Signed-off-by: Arvind Sudarsanam <[email protected]>

llvmbot added the clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' label Apr 15, 2025

sarnex approved these changes Apr 16, 2025

View reviewed changes

sarnex merged commit 6cfec29 into llvm:main Apr 16, 2025
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Offload][SYCL] Refactor OffloadKind implementation #135809

[Offload][SYCL] Refactor OffloadKind implementation #135809

asudarsa commented Apr 15, 2025

llvmbot commented Apr 15, 2025 •

edited

Loading

jhuber6 Apr 15, 2025

asudarsa Apr 15, 2025

asudarsa Apr 15, 2025

jhuber6 Apr 15, 2025

asudarsa Apr 15, 2025

jhuber6 Apr 15, 2025

asudarsa Apr 15, 2025

llvm-ci commented Apr 16, 2025

asudarsa commented Apr 16, 2025

[Offload][SYCL] Refactor OffloadKind implementation #135809

[Offload][SYCL] Refactor OffloadKind implementation #135809

Conversation

asudarsa commented Apr 15, 2025

llvmbot commented Apr 15, 2025 • edited Loading

jhuber6 Apr 15, 2025

Choose a reason for hiding this comment

asudarsa Apr 15, 2025

Choose a reason for hiding this comment

asudarsa Apr 15, 2025

Choose a reason for hiding this comment

jhuber6 Apr 15, 2025

Choose a reason for hiding this comment

asudarsa Apr 15, 2025

Choose a reason for hiding this comment

jhuber6 Apr 15, 2025

Choose a reason for hiding this comment

asudarsa Apr 15, 2025

Choose a reason for hiding this comment

llvm-ci commented Apr 16, 2025

asudarsa commented Apr 16, 2025

llvmbot commented Apr 15, 2025 •

edited

Loading