Skip to content

[SYCL] Gracefully handle unknown device #11254

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

Merged
merged 5 commits into from
Nov 28, 2023
Merged

[SYCL] Gracefully handle unknown device #11254

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
452f9a2
[SYCL] Gracefully handle unknown device
al42and Sep 21, 2023
2f197d8
Merge remote-tracking branch 'origin/sycl' into fix-8112
al42and Nov 27, 2023
4dcf5b6
Add test
al42and Nov 27, 2023
3d16390
Use AMDGCN target instead of NVPTX for testing
al42and Nov 28, 2023
081d821
Add nogpulib flag to tests
al42and Nov 28, 2023
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
2 changes: 1 addition & 1 deletion clang/lib/Driver/ToolChains/Clang.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5465,7 +5465,7 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
Triple.getSubArch() == llvm::Triple::SPIRSubArch_gen) ||
Triple.isNVPTX() || Triple.isAMDGCN()) {
StringRef Device = JA.getOffloadingArch();
if (!Device.empty()) {
if (!Device.empty() && !SYCL::gen::getGenDeviceMacro(Device).empty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a test for this circumstance? Also, would it be reasonable to emit some kind of diagnostic that the macro isn't being generated due to an issue with the device value?

Copy link
Contributor Author

@al42and al42and Sep 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Should we add a test for this circumstance?

Could you advise on the best place to put a test? clang/test/Driver/sycl-oneapi-gpu.cpp?

Also, would it be reasonable to emit some kind of diagnostic that the macro isn't being generated due to an issue with the device value?

I don't think macro alone warrants this. It's prone to false positives (what if the code don't use these macros at all?), and occurs at the wrong place (it's not the problem of the user that they have gfx1036, it's the problem of the developer who wrote code relying a macro __SYCL_TARGET_AMD_GPU_GFX1036__). Warning about unsupported macros in the code (__SYCL_TARGET_* will never be set) independently of the target architecture could will be better: it will be emitted if and only if there is an actual bug in the code, but this is already covered by -Wundef (since, if the macro is supported, it will be defined to 0, https://github.com/intel/llvm/blob/de92299c2c09d626dcbc633c83e259d828220d03/sycl/doc/design/DeviceIf.md).

We could, more generally, warn about "unsupported" devices, the way Clang warns about "unsupported" CUDA versions (-Wunknown-cuda-version). I don't think it fits the scope of this PR, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @al42and . If you read the DeviceIf.md, it is quite clear IMO:

"These macros are an internal implementation detail, so they should not be documented to users, and user code should not make use of them."

@al42and I think the answer to your question is that the appropriate place to place such a test is in clang/test/Driver: I guess you just want something similar to this: https://github.com/JackAKirk/llvm/blob/2b4b45af5a1ae0d23bcb632cc4588faecedc1956/clang/test/Driver/sycl-fno-libspirv-warn.cpp

except you want to check there are no warnings or errors when compiling for a device that e.g doesn't have a macro assigned but is supported by the driver.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

except you want to check there are no warnings or errors when compiling for a device that e.g doesn't have a macro assigned but is supported by the driver.

That's going to be an unreliable test and in need of regular updates. Hopefully, such architectures will eventually get assigned a flag :)

I think a better test is to check that when compiling for gfx0000 or sm_00 we get a reasonable error instead of an ICE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think a better test is to check that when compiling for gfx0000 or sm_00 we get a reasonable error instead of an ICE.

Unfortunately, the validity of the architecture is checked before the broken compiler pass is invoked, so this idea will not work.

Instead, I'm using an old architecture (e.g., sm_30), that is valid but is not and will not be supported.

Copy link
Contributor Author

@al42and al42and Nov 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Looks like NVIDIA device libraries are not as reliable as I thought they are. Changed the test to use an old AMD architecture (gfx600, supported by LLVM, not listed in clang/lib/Driver/ToolChains/SYCL.cpp) instead.

Copy link
Contributor Author

@al42and al42and Nov 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

These tests are harder than I expected 😅 Hopefully I'll get "Build + LIT" to pass this time with 081d821.

The timeout of Basic/vector/int-convert.cpp in e2e / Intel GEN12 Graphics with Level Zero is unrelated: #12011

Macro = "-D";
Macro += SYCL::gen::getGenDeviceMacro(Device);
}
Expand Down
8 changes: 8 additions & 0 deletions clang/test/Driver/sycl-unsupported-arch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/// Verify that compiler passes are correctly determined
// RUN: %clangxx -fsycl -fsycl-targets=amdgcn-amd-amdhsa -Xsycl-target-backend=amdgcn-amd-amdhsa --offload-arch=gfx600 -nogpulib -c -ccc-print-phases %s 2>&1 | FileCheck %s --check-prefix=CHECK-PHASES
// CHECK-PHASES: offload, "device-sycl (amdgcn-amd-amdhsa:gfx600)"

/// Verify that preprocessor works (#8112)
// RUN: %clangxx -fsycl -fsycl-targets=amdgcn-amd-amdhsa -Xsycl-target-backend=amdgcn-amd-amdhsa --offload-arch=gfx600 -nogpulib -E %s 2>&1 | FileCheck %s --check-prefix=CHECK-PREPROCESSOR
// CHECK-PREPROCESSOR: // __CLANG_OFFLOAD_BUNDLE____START__ sycl-amdgcn-amd-amdhsa-gfx600