Skip to content

[SYCL] Fix handling of comma separated arch value when calling clang-offload-packager #20130

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: sycl
Choose a base branch
from
Open

[SYCL] Fix handling of comma separated arch value when calling clang-offload-packager #20130

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9329697
[SYCL] Fix handling of comma separated arch value when calling clang-…
jzc Sep 18, 2025
3c38506
Address review comments
jzc Oct 2, 2025
1945480
Merge remote-tracking branch 'intel/sycl' into comma-tmp
jzc Oct 2, 2025
c8c9cb6
Add -Xsycl-target-linker to test
jzc Oct 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions clang/lib/Driver/ToolChains/Clang.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10314,9 +10314,21 @@ void OffloadPackager::ConstructJob(Compilation &C, const JobAction &JA,
File = C.getArgs().MakeArgString("@" + File);

StringRef Arch;
std::string TransformedArch;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, add a comment with the requirement for TransformedArch to outlive Arch. It's not obvious from the variable declaration.

if (OffloadAction->getOffloadingArch()) {
if (TC->getTripleString() == "spir64_gen-unknown-unknown") {
Arch = mapIntelGPUArchName(OffloadAction->getOffloadingArch());
// When compiling like -fsycl-targets=spir64_gen -Xsycl-target-backend
// "-device pvc,bdw", the offloading arch will be "pvc,bdw", which
// contains a comma. We need to transform it to "arch=pvc,arch=bdw" when
// passing to clang-offload-packager.
Arch = OffloadAction->getOffloadingArch();
SmallVector<StringRef> Archs;
Arch.split(Archs, ',');
for (StringRef &A : Archs) {
A = mapIntelGPUArchName(A);
}
TransformedArch = llvm::join(Archs, ",arch=");
Arch = TransformedArch;
} else {
Arch = OffloadAction->getOffloadingArch();
}
Expand Down Expand Up @@ -10366,7 +10378,9 @@ void OffloadPackager::ConstructJob(Compilation &C, const JobAction &JA,
AL += " ";
AL += A;
}
Parts.emplace_back(C.getArgs().MakeArgString(Twine(Opt) + AL));
for (StringRef Split : llvm::split(AL, ',')) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add a comment describing what this newly added code does or update the above existing comment.

Parts.emplace_back(C.getArgs().MakeArgString(Twine(Opt) + Split));
}
};
const ArgList &Args =
C.getArgsForToolChain(nullptr, StringRef(), Action::OFK_SYCL);
Expand Down
8 changes: 7 additions & 1 deletion clang/test/Driver/sycl-ftarget-compile-fast.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,14 @@
// RUN: -fsycl-targets=spir64_gen -ftarget-compile-fast %s 2>&1 \
// RUN: | FileCheck -check-prefix=TARGET_COMPILE_FAST_GEN %s


// Due to how clang-offload-packager works, if we want a value in a key-value pair to
// have a comma, we need to specify the key twice, once for each part of the value
// separated by the comma. clang-offload-packager will then combine the two parts into a single
// value with a comma in between.
// TARGET_COMPILE_FAST_GEN: clang-offload-packager
// TARGET_COMPILE_FAST_GEN: compile-opts={{.*}}-options -igc_opts 'PartitionUnit=1,SubroutineThreshold=50000'
// TARGET_COMPILE_FAST_GEN: compile-opts={{.*}}-options -igc_opts 'PartitionUnit=1
// TARGET_COMPILE_FAST_GEN-SAME: compile-opts=SubroutineThreshold=50000'

// RUN: %clang -### -target x86_64-unknown-linux-gnu -fsycl --offload-new-driver \
// RUN: -ftarget-compile-fast %s 2>&1 \
Expand Down
17 changes: 16 additions & 1 deletion clang/test/Driver/sycl-offload-new-driver.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@
// RUN: %clangxx -fsycl -### -fsycl-targets=spir64_gen --offload-new-driver \
// RUN: -Xsycl-target-backend=spir64_gen "-device pvc,bdw" %s 2>&1 \
// RUN: | FileCheck -check-prefix COMMA_FILE %s
// COMMA_FILE: clang-offload-packager{{.*}} "--image=file={{.*}}pvc@bdw{{.*}},triple=spir64_gen-unknown-unknown,arch=pvc,bdw,kind=sycl,compile-opts=-device_options pvc -ze-intel-enable-auto-large-GRF-mode"
// COMMA_FILE: clang-offload-packager{{.*}} "--image=file={{.*}}pvc@bdw{{.*}},triple=spir64_gen-unknown-unknown,arch=pvc,arch=bdw,kind=sycl,compile-opts=-device_options pvc -ze-intel-enable-auto-large-GRF-mode"

/// Verify the arch value for the packager is populated with different
/// scenarios for spir64_gen
Expand All @@ -211,6 +211,21 @@
// RUN: | FileCheck -check-prefix ARCH_CHECK %s
// ARCH_CHECK: clang-offload-packager{{.*}} "--image=file={{.*}}triple=spir64_gen-unknown-unknown,arch=bdw,kind=sycl{{.*}}"

// Verify when a comma-separated list of architectures is provided in -device, they are
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the behavior if we pass a non-comma separated list, say a space or semi-colon separated list?
Do we get an error/warning?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this question addressed?

// passed to clang-offload-packager correctly
Copy link
Contributor

@srividya-sundaram srividya-sundaram Sep 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--offload-arch also supports comma-separated list of arch values.
Could you please add a test case covering that scenario as well?
Example: -Xsycl-target-backend --offload-arch=sm_80,sm_90

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, -Xsycl-target-backend and -Xsycl-target-backend= are handled individually in Driver code.
Is there a test case added for this case?

// RUN: %clangxx -fsycl -### -fsycl-targets=spir64_gen --offload-new-driver \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, a bit more complex test case such as:
-fsycl-targets=nvptx64-nvidia-cuda,amdgcn-amd-amdhsa,spir64_gen -Xsycl-target-backend=amdgcn-amd-amdhsa --offload-arch=gfx908,gfx1010 -Xsycl-target-backend=nvptx64-nvidia-cuda --offload-arch=sm_87,sm_88,sm_89 -Xsycl-target-backend=spir64_gen "-ze-intel-enable-auto-large-GRF-mode -device pvc,bdw"

// RUN: -Xsycl-target-backend "-device pvc,bdw" %s 2>&1 \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another suggestion is to document this behavior in the help text or documentation instead of adding diagnostic in the code.

// RUN: | FileCheck -check-prefix MULTI_ARCH %s
// MULTI_ARCH: clang-offload-packager{{.*}} "--image=file={{.*}}triple=spir64_gen-unknown-unknown,arch=pvc,arch=bdw,kind=sycl
// MULTI_ARCH-SAME: compile-opts=-device_options pvc -ze-intel-enable-auto-large-GRF-mode -device pvc,compile-opts=bdw"

// Verify that the driver correctly handles link-opt and compile-opt values with commas
// RUN: %clangxx -fsycl -### -fsycl-targets=spir64_gen --offload-new-driver \
// RUN: -Xsycl-target-backend "-device bdw -FOO a,b" \
// RUN: -Xsycl-target-linker "-BAR x,y" %s 2>&1 \
// RUN: | FileCheck -check-prefix COMMA_OPTS %s
// COMMA_OPTS: clang-offload-packager{{.*}} "--image=file={{.*}}triple=spir64_gen-unknown-unknown,arch=bdw,kind=sycl,compile-opts=-device bdw -FOO a,compile-opts=b,link-opts=-BAR x,link-opts=y"

/// Verify that --cuda-path is passed to clang-linker-wrapper for SYCL offload
// RUN: %clangxx -fsycl -### -fsycl-targets=nvptx64-nvidia-cuda -fno-sycl-libspirv \
// RUN: --cuda-gpu-arch=sm_20 --cuda-path=%S/Inputs/CUDA_80/usr/local/cuda %s \
Expand Down
Loading