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

[SYCL]Fix SYCL target triple check for NVidia targets. #14505

Closed
wants to merge 4 commits into from
Closed

[SYCL]Fix SYCL target triple check for NVidia targets. #14505

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
af850b6
[SYCL]Fix SYCL target triple check for NVidia targets.
srividya-sundaram Jul 9, 2024
7956296
Remove else block.
srividya-sundaram Jul 9, 2024
00f95f1
Update tests with valid target triple for NVidia targets.
srividya-sundaram Jul 9, 2024
3976955
Update tests.
srividya-sundaram Jul 10, 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
22 changes: 13 additions & 9 deletions clang/lib/Driver/Driver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -806,24 +806,28 @@ Driver::OpenMPRuntimeKind Driver::getOpenMPRuntime(const ArgList &Args) const {
return RT;
}

static bool isValidSYCLTriple(llvm::Triple T) {
// NVPTX is valid for SYCL.
if (T.isNVPTX())
static bool isValidSYCLTriple(llvm::Triple TargetTriple) {
// Check for invalid SYCL device triple values for NVidia GPU targets.
// nvptx64-nvidia-cuda is the valid triple for NVidia targets.
if (TargetTriple.getArchTypeName(TargetTriple.getArch()) == "nvptx64" &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (TargetTriple.getArchTypeName(TargetTriple.getArch()) == "nvptx64" &&
if (TargetTriple.isNVPTX() &&

Why do we need to do string comparisons?

TargetTriple.getVendorTypeName(TargetTriple.getVendor()) == "nvidia" &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Once again, I would prefer to avoid string comparison. Target.getVendor() == Triple::VendorType::NVIDIA should be enough

TargetTriple.getOSTypeName(TargetTriple.getOS()) == "cuda" &&
!TargetTriple.hasEnvironment())
return true;

// AMDGCN is valid for SYCL
if (T.isAMDGCN())
if (TargetTriple.isAMDGCN())
return true;

// Check for invalid SYCL device triple values.
// Non-SPIR/SPIRV arch.
if (!T.isSPIROrSPIRV())
if (!TargetTriple.isSPIROrSPIRV())
return false;
// SPIR/SPIRV arch, but has invalid SubArch for AOT.
StringRef A(T.getArchName());
if (T.getSubArch() == llvm::Triple::NoSubArch &&
((T.getArch() == llvm::Triple::spir && A != "spir") ||
(T.getArch() == llvm::Triple::spir64 && A != "spir64")))
StringRef A(TargetTriple.getArchName());
if (TargetTriple.getSubArch() == llvm::Triple::NoSubArch &&
((TargetTriple.getArch() == llvm::Triple::spir && A != "spir") ||
(TargetTriple.getArch() == llvm::Triple::spir64 && A != "spir64")))
return false;
return true;
}
Expand Down
2 changes: 1 addition & 1 deletion clang/test/Driver/sycl-libspirv.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// UNSUPPORTED: system-windows

// RUN: %clangxx -### -std=c++11 -target x86_64-unknown-linux-gnu -fsycl \
// RUN: -fsycl-targets=nvptx64-nvidia-nvcl --cuda-path=%S/Inputs/CUDA/usr/local/cuda \
// RUN: -fsycl-targets=nvptx64-nvidia-cuda --cuda-path=%S/Inputs/CUDA/usr/local/cuda \
// RUN: -fsycl-libspirv-path=%S/Inputs/SYCL/libspirv.bc %s 2>&1 \
// RUN: | FileCheck %s
// CHECK: {{.*}} "-mlink-builtin-bitcode" "{{.*}}libspirv.bc" {{.*}}
4 changes: 2 additions & 2 deletions clang/test/Driver/sycl-offload.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -668,7 +668,7 @@
/// ###########################################################################

// Check if valid bound arch behaviour occurs when compiling for spir-v,nvidia-gpu, and amd-gpu
// RUN: %clang -target x86_64-unknown-linux-gnu -fsycl -fno-sycl-instrument-device-code -fno-sycl-device-lib=all -fsycl-targets=spir64,nvptx-nvidia-cuda,amdgcn-amd-amdhsa -Xsycl-target-backend=nvptx-nvidia-cuda --offload-arch=sm_75 -Xsycl-target-backend=amdgcn-amd-amdhsa --offload-arch=gfx908 -ccc-print-phases %s 2>&1 \
// RUN: %clang -target x86_64-unknown-linux-gnu -fsycl -fno-sycl-instrument-device-code -fno-sycl-device-lib=all -fsycl-targets=spir64,nvptx64-nvidia-cuda,amdgcn-amd-amdhsa -Xsycl-target-backend=nvptx64-nvidia-cuda --offload-arch=sm_75 -Xsycl-target-backend=amdgcn-amd-amdhsa --offload-arch=gfx908 -ccc-print-phases %s 2>&1 \
// RUN: | FileCheck -check-prefix=CHK-PHASE-MULTI-TARG-SPIRV-NVIDIA-AMD %s
// CHK-PHASE-MULTI-TARG-SPIRV-NVIDIA-AMD: 0: input, "[[INPUT:.+\.c]]", c++, (host-sycl)
// CHK-PHASE-MULTI-TARG-SPIRV-NVIDIA-AMD: 1: append-footer, {0}, c++, (host-sycl)
Expand Down Expand Up @@ -702,7 +702,7 @@
// CHK-PHASE-MULTI-TARG-SPIRV-NVIDIA-AMD: 29: foreach, {25, 28}, cuda-fatbin, (device-sycl, sm_75)
// CHK-PHASE-MULTI-TARG-SPIRV-NVIDIA-AMD: 30: file-table-tform, {24, 29}, tempfiletable, (device-sycl, sm_75)
// CHK-PHASE-MULTI-TARG-SPIRV-NVIDIA-AMD: 31: clang-offload-wrapper, {30}, object, (device-sycl, sm_75)
// CHK-PHASE-MULTI-TARG-SPIRV-NVIDIA-AMD: 32: offload, "device-sycl (nvptx-nvidia-cuda:sm_75)" {31}, object
// CHK-PHASE-MULTI-TARG-SPIRV-NVIDIA-AMD: 32: offload, "device-sycl (nvptx64-nvidia-cuda:sm_75)" {31}, object
// CHK-PHASE-MULTI-TARG-SPIRV-NVIDIA-AMD: 33: linker, {5}, ir, (device-sycl, gfx908)
// CHK-PHASE-MULTI-TARG-SPIRV-NVIDIA-AMD: 34: sycl-post-link, {33}, ir, (device-sycl, gfx908)
// CHK-PHASE-MULTI-TARG-SPIRV-NVIDIA-AMD: 35: file-table-tform, {34}, ir, (device-sycl, gfx908)
Expand Down
12 changes: 12 additions & 0 deletions clang/test/Driver/sycl-oneapi-gpu-nvidia.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,18 @@
// RUN: FileCheck %s --check-prefix=BAD_NVIDIA_INPUT
// BAD_NVIDIA_INPUT: error: SYCL target is invalid: 'nvidia_gpu_bad'

// RUN: not %clangxx -c -fsycl -fsycl-targets=nvptx64-nvidia-cuda-sycl -### %s 2>&1 | \
// RUN: FileCheck %s --check-prefix=BAD_TARGET_TRIPLE_ENV
// RUN: not %clang_cl -c -fsycl -fsycl-targets=nvptx64-nvidia-cuda-sycl -### %s 2>&1 | \
// RUN: FileCheck %s --check-prefix=BAD_TARGET_TRIPLE_ENV
// BAD_TARGET_TRIPLE_ENV: error: SYCL target is invalid: 'nvptx64-nvidia-cuda-sycl'

// RUN: not %clangxx -c -fsycl -fsycl-targets=nvptx64-nvidia-cuda1 -### %s 2>&1 | \
// RUN: FileCheck %s --check-prefix=BAD_TARGET_TRIPLE
// RUN: not %clang_cl -c -fsycl -fsycl-targets=nvptx64-nvidia-cuda1 -### %s 2>&1 | \
// RUN: FileCheck %s --check-prefix=BAD_TARGET_TRIPLE
// BAD_TARGET_TRIPLE: error: SYCL target is invalid: 'nvptx64-nvidia-cuda1'

/// Test for proper creation of fat object
// RUN: %clangxx -c -fsycl -nocudalib -fsycl-targets=nvidia_gpu_sm_50 \
// RUN: -fsycl-libspirv-path=%S/Inputs/SYCL/libspirv.bc \
Expand Down
86 changes: 43 additions & 43 deletions llvm/lib/TargetParser/Triple.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -649,49 +649,49 @@ static Triple::VendorType parseVendor(StringRef VendorName) {

static Triple::OSType parseOS(StringRef OSName) {
return StringSwitch<Triple::OSType>(OSName)
.StartsWith("darwin", Triple::Darwin)
.StartsWith("dragonfly", Triple::DragonFly)
.StartsWith("freebsd", Triple::FreeBSD)
.StartsWith("fuchsia", Triple::Fuchsia)
.StartsWith("ios", Triple::IOS)
.StartsWith("kfreebsd", Triple::KFreeBSD)
.StartsWith("linux", Triple::Linux)
.StartsWith("lv2", Triple::Lv2)
.StartsWith("macos", Triple::MacOSX)
.StartsWith("netbsd", Triple::NetBSD)
.StartsWith("openbsd", Triple::OpenBSD)
.StartsWith("solaris", Triple::Solaris)
.StartsWith("uefi", Triple::UEFI)
.StartsWith("win32", Triple::Win32)
.StartsWith("windows", Triple::Win32)
.StartsWith("zos", Triple::ZOS)
.StartsWith("haiku", Triple::Haiku)
.StartsWith("rtems", Triple::RTEMS)
.StartsWith("nacl", Triple::NaCl)
.StartsWith("aix", Triple::AIX)
.StartsWith("cuda", Triple::CUDA)
.StartsWith("nvcl", Triple::NVCL)
.StartsWith("amdhsa", Triple::AMDHSA)
.StartsWith("ps4", Triple::PS4)
.StartsWith("ps5", Triple::PS5)
.StartsWith("elfiamcu", Triple::ELFIAMCU)
.StartsWith("tvos", Triple::TvOS)
.StartsWith("watchos", Triple::WatchOS)
.StartsWith("bridgeos", Triple::BridgeOS)
.StartsWith("driverkit", Triple::DriverKit)
.StartsWith("xros", Triple::XROS)
.StartsWith("visionos", Triple::XROS)
.StartsWith("mesa3d", Triple::Mesa3D)
.StartsWith("amdpal", Triple::AMDPAL)
.StartsWith("hermit", Triple::HermitCore)
.StartsWith("hurd", Triple::Hurd)
.StartsWith("wasi", Triple::WASI)
.StartsWith("emscripten", Triple::Emscripten)
.StartsWith("shadermodel", Triple::ShaderModel)
.StartsWith("liteos", Triple::LiteOS)
.StartsWith("serenity", Triple::Serenity)
.StartsWith("vulkan", Triple::Vulkan)
.Default(Triple::UnknownOS);
.StartsWith("darwin", Triple::Darwin)
.StartsWith("dragonfly", Triple::DragonFly)
.StartsWith("freebsd", Triple::FreeBSD)
.StartsWith("fuchsia", Triple::Fuchsia)
.StartsWith("ios", Triple::IOS)
.StartsWith("kfreebsd", Triple::KFreeBSD)
.StartsWith("linux", Triple::Linux)
.StartsWith("lv2", Triple::Lv2)
.StartsWith("macos", Triple::MacOSX)
.StartsWith("netbsd", Triple::NetBSD)
.StartsWith("openbsd", Triple::OpenBSD)
.StartsWith("solaris", Triple::Solaris)
.StartsWith("uefi", Triple::UEFI)
.StartsWith("win32", Triple::Win32)
.StartsWith("windows", Triple::Win32)
.StartsWith("zos", Triple::ZOS)
.StartsWith("haiku", Triple::Haiku)
.StartsWith("rtems", Triple::RTEMS)
.StartsWith("nacl", Triple::NaCl)
.StartsWith("aix", Triple::AIX)
.Case("cuda", Triple::CUDA)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm hesitant to accept this change at intel/llvm. If you can get this accepted to the upstream LLVM, then it is fine.

This change is at least inconsistent with handling of other OSes. For example, clang will still accept nvptx-nvidia-nvclmyown as a valid triple for OpenCL on NVIDIA, even though the spelling contains some extra myown.

My main concern is that no one prohibits using other things from intel/llvm besides SYCL (and I know for sure that folks are using OpenMP for example) and therefore this change could break someone's workflow that we don't know about. Since we don't mark our customizations to the upstream codebase in any way, this change could easily get lost, cause conflicts or be hard to find during debugging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @AlexeySachkov

Currently, when we pass an invalid target triple value to fsycl-targets for NVidia targets , we get the following error:

fatal error: error in backend: Cannot select: intrinsic %llvm.nvvm.implicit.offset
llvm-foreach:
clang++: error: clang frontend command failed with exit code 70 (use -v to see invocation)

This error is non-intuitive and does not clearly describe what the problem is.

Currently, in the driver code base, there is no check to verify if the target triple string passed matches the exact value documented - nvptx64-nvidia-cuda
We only check if the ‘architecture’ component has ‘nvptx64’ and return that as a valid string.
Example: nvptx64-badVendor-badOS would still be passed as a valid triple to the toolchain and we end up getting an error.

For OpenMP, they do allow partial/shortened triple strings with empty Vendor or/and OS components. The following triples are all valid. Please see this function
nvptx64
nvptx64-nvidia
nvptx64--cuda

As for this change: .Case("cuda", Triple::CUDA)
When we just check using StartsWith , for an example like this - nvptx64-nvidia-cuda123,
the 123 get parsed as the OS Major version but the triple still gets rejected because cuda123 is not a valid OS component for offloading SYCL to nvptx backend.

I can remove the string comparisons as parseArch() , parseVendor() and parseOS() all use .Case match for Nvidia targets.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the best we can do is check for Triple::CUDA for the OS and not try to be strict about it. Sure we would be allowing OS values of cudaBadString, but given how doing so is consistent with other OS settings I'm hard pressed to deviate. If the failure occurs down the line when using cudaBadString, there may be an issue later in the compilation when using the triple that isn't using the known OS check.

Copy link
Contributor

Choose a reason for hiding this comment

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

This error is non-intuitive and does not clearly describe what the problem is.

I agree, but I still don't think that the suggested customization is right in context of deviation from the community code, further maintenance effort and potential unexpected/unwanted side effects.

For OpenMP, they do allow partial/shortened triple strings with empty Vendor or/and OS components.

We should adopt this in sycl and it will automatically reduce the amount of uses of longer triple that is prone to errors. Moreover, -fsycl-targets will eventually move to --arch that accepts a target name and not a triple, resolving this issue all together.

.StartsWith("nvcl", Triple::NVCL)
.StartsWith("amdhsa", Triple::AMDHSA)
.StartsWith("ps4", Triple::PS4)
.StartsWith("ps5", Triple::PS5)
.StartsWith("elfiamcu", Triple::ELFIAMCU)
.StartsWith("tvos", Triple::TvOS)
.StartsWith("watchos", Triple::WatchOS)
.StartsWith("bridgeos", Triple::BridgeOS)
.StartsWith("driverkit", Triple::DriverKit)
.StartsWith("xros", Triple::XROS)
.StartsWith("visionos", Triple::XROS)
.StartsWith("mesa3d", Triple::Mesa3D)
.StartsWith("amdpal", Triple::AMDPAL)
.StartsWith("hermit", Triple::HermitCore)
.StartsWith("hurd", Triple::Hurd)
.StartsWith("wasi", Triple::WASI)
.StartsWith("emscripten", Triple::Emscripten)
.StartsWith("shadermodel", Triple::ShaderModel)
.StartsWith("liteos", Triple::LiteOS)
.StartsWith("serenity", Triple::Serenity)
.StartsWith("vulkan", Triple::Vulkan)
.Default(Triple::UnknownOS);
}

static Triple::EnvironmentType parseEnvironment(StringRef EnvironmentName) {
Expand Down
Loading