Skip to content
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

Jump to bottom

[Driver][SYCL] Handle invalid characters from device in temp files #14563

Merged
merged 4 commits into from
Jul 26, 2024
Merged

[Driver][SYCL] Handle invalid characters from device in temp files #14563

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0636c88
[Driver][SYCL] Handle invalid characters from device in temp files
mdtoguchi Jul 12, 2024
b07237e
Merge remote-tracking branch 'intel_llvm/sycl' into device-temp-name-fix
mdtoguchi Jul 24, 2024
7eb0095
Add additional conversion for commas
mdtoguchi Jul 25, 2024
c6d1f22
Merge remote-tracking branch 'intel_llvm/sycl' into device-temp-name-fix
mdtoguchi Jul 25, 2024
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
8 changes: 6 additions & 2 deletions clang/lib/Driver/Driver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9417,10 +9417,14 @@ const char *Driver::GetNamedOutputPath(Compilation &C, const JobAction &JA,
StringRef OffloadingPrefix) const {
std::string BoundArch = OrigBoundArch.str();
if (is_style_windows(llvm::sys::path::Style::native)) {
// BoundArch may contains ':', which is invalid in file names on Windows,
// therefore replace it with '%'.
// BoundArch may contain ':' or '*', which is invalid in file names on
// Windows, therefore replace it with '@'.
std::replace(BoundArch.begin(), BoundArch.end(), ':', '@');
std::replace(BoundArch.begin(), BoundArch.end(), '*', '@');
}
// BoundArch may contain ',', which may create strings that interfere with
// the StringMap for the clang-offload-packager input values.
std::replace(BoundArch.begin(), BoundArch.end(), ',', '@');

llvm::PrettyStackTraceString CrashInfo("Computing output path");
// Output to a user requested destination?
Expand Down
7 changes: 7 additions & 0 deletions 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 @@ -181,3 +181,10 @@
// RUN: -fsycl-embed-ir %s 2>&1 \
// RUN: | FileCheck -check-prefix CHECK_EMBED_IR %s
// CHECK_EMBED_IR: clang-linker-wrapper{{.*}} "-sycl-embed-ir"

/// Verify the filename being passed to the packager does not contain commas
/// that are used in -device settings.
// 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"
Copy link
Contributor

Choose a reason for hiding this comment

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

There is an additional comma here arch=pvc,bdw,
Is this expected or is this the issue you were referring to in your comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As of now, it is expected and we will resolve with discussions with Arvind.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you can create a GitHub issue and link that to this PR.

20 changes: 20 additions & 0 deletions clang/test/Driver/sycl-windows-device-filename.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// Test valid file names from -device values for GPU
// REQUIRES: system-windows

// RUN: %clang -### --target=x86_64-pc-windows-msvc -fsycl \
// RUN: -fsycl-targets=spir64_gen --offload-new-driver \
// RUN: -Xsycl-target-backend "-device arch1:arch2" %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.

What is the behavior when the device arch values are comma separated or space separated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The strings are not modified. Now, using commas is fine for file names AFAIK, but this could throw a wrench into the arch settings for the clang-offload-packager.

@asudarsa, any thoughts on how we can handle comma separated arch values when passed in like -device arch1,arch2,arch3 and is embedded in the clang-offload-packager image-file stringmap? As the values are comma separated I think it would be parsed incorrectly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@srividya-sundaram, I have added some additional changes to convert commas. Additional side discussions to be had with @asudarsa to resolve other issues with the packager.

// RUN: | FileCheck %s -check-prefix=CHECK_COLON

// CHECK_COLON: sycl-windows-device-filename-arch1@arch2
// CHECK_COLON: arch=arch1:arch2
// CHECK_COLON-NOT: sycl-windows-device-filename-arch1:arch2

// RUN: %clang -### --target=x86_64-pc-windows-msvc -fsycl \
// RUN: -fsycl-targets=spir64_gen --offload-new-driver \
// RUN: -Xsycl-target-backend "-device *" %s 2>&1 \
// RUN: | FileCheck %s -check-prefix=CHECK_STAR

// CHECK_STAR: sycl-windows-device-filename-@
// CHECK_STAR: arch=*
// CHECK_STAR-NOT: sycl-windows-device-filename-*
Loading