-
Notifications
You must be signed in to change notification settings - Fork 12k
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.
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
[Driver] Test ignored target-specific options for AMDGPU/NVPTX #79222
[Driver] Test ignored target-specific options for AMDGPU/NVPTX #79222
Conversation
Created using spr 1.3.4
@llvm/pr-subscribers-clang-driver Author: Fangrui Song (MaskRay) ChangesFix missing test coverage after #70740 #70760 Full diff: https://github.com/llvm/llvm-project/pull/79222.diff 1 Files Affected:
diff --git a/clang/test/Driver/unsupported-option-gpu.c b/clang/test/Driver/unsupported-option-gpu.c
new file mode 100644
index 000000000000000..5713dbbfc7ae4db
--- /dev/null
+++ b/clang/test/Driver/unsupported-option-gpu.c
@@ -0,0 +1,7 @@
+/// Some target-specific options are ignored for GPU, so %clang exits with code 0.
+// DEFINE: %{gpu_opts} = --cuda-gpu-arch=sm_60 --cuda-path=%S/Inputs/CUDA/usr/local/cuda --no-cuda-version-check
+// DEFINE: %{check} = %clang -### -c %{gpu_opts} -mcmodel=medium %s
+// RUN: %{check} -fbasic-block-sections=all
+
+// REDEFINE: %{gpu_opts} = -x hip --rocm-path=%S/Inputs/rocm -nogpulib
+// RUN: %{check}
|
@llvm/pr-subscribers-clang Author: Fangrui Song (MaskRay) ChangesFix missing test coverage after #70740 #70760 Full diff: https://github.com/llvm/llvm-project/pull/79222.diff 1 Files Affected:
diff --git a/clang/test/Driver/unsupported-option-gpu.c b/clang/test/Driver/unsupported-option-gpu.c
new file mode 100644
index 000000000000000..5713dbbfc7ae4db
--- /dev/null
+++ b/clang/test/Driver/unsupported-option-gpu.c
@@ -0,0 +1,7 @@
+/// Some target-specific options are ignored for GPU, so %clang exits with code 0.
+// DEFINE: %{gpu_opts} = --cuda-gpu-arch=sm_60 --cuda-path=%S/Inputs/CUDA/usr/local/cuda --no-cuda-version-check
+// DEFINE: %{check} = %clang -### -c %{gpu_opts} -mcmodel=medium %s
+// RUN: %{check} -fbasic-block-sections=all
+
+// REDEFINE: %{gpu_opts} = -x hip --rocm-path=%S/Inputs/rocm -nogpulib
+// RUN: %{check}
|
@@ -0,0 +1,7 @@ | |||
/// Some target-specific options are ignored for GPU, so %clang exits with code 0. | |||
// DEFINE: %{gpu_opts} = --cuda-gpu-arch=sm_60 --cuda-path=%S/Inputs/CUDA/usr/local/cuda --no-cuda-version-check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these defines seem overkill and harder to read compared to just duplicating the command line twice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think gpu_opts
can be merged into %{check}
. %{check}
should be kept as it will become longer soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the purpose of warning checking, we can drop this altogether and use -nogpuinc -nogpulib
. Whatever default GPU we end up targeting is also irrelevant.
@@ -0,0 +1,7 @@ | |||
/// Some target-specific options are ignored for GPU, so %clang exits with code 0. | |||
// DEFINE: %{gpu_opts} = --cuda-gpu-arch=sm_60 --cuda-path=%S/Inputs/CUDA/usr/local/cuda --no-cuda-version-check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the purpose of warning checking, we can drop this altogether and use -nogpuinc -nogpulib
. Whatever default GPU we end up targeting is also irrelevant.
/// Some target-specific options are ignored for GPU, so %clang exits with code 0. | ||
// DEFINE: %{gpu_opts} = --cuda-gpu-arch=sm_60 --cuda-path=%S/Inputs/CUDA/usr/local/cuda --no-cuda-version-check | ||
// DEFINE: %{check} = %clang -### -c %{gpu_opts} -mcmodel=medium %s | ||
// RUN: %{check} -fbasic-block-sections=all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may want to either rename the file into .cu
or use -x cuda
. Otherwise you're running a plain C compilation.
Also, what exactly are we checking here? With -###
CC1 sub-compilations do not run and, I think, that's where unsupported options were triggering warning/errors.
You may want to do two actual compilations, one with --cuda-host-only
and one with --cuda-device-only -s
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added -x cuda
. The test is to show we don't get an error (err_drv_unsupported_opt_for_target
) when compiling for x86_64 using a device (AMDGPU/NVPTX) when certain target-specified options are specified.
I am not familiar with offloading but specifying --cuda-host-only
would defeat the purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Offloading compilation for these single-source languages pretty much just combines one "host" compilation job with N "Device" compilation jobs. Doing --offload-device-only
and --offload-host-only
simply does one part of that. There's probably some flags that behave differently depending on which end you're compiling on, so maybe it would be useful for separating that behavior if needed.
Created using spr 1.3.4
// DEFINE: %{check} = %clang -### -c %{gpu_opts} -mcmodel=medium %s | ||
// RUN: %{check} -fbasic-block-sections=all | ||
|
||
// REDEFINE: %{gpu_opts} = -x hip --rocm-path=%S/Inputs/rocm -nogpulib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably include -nogpuinc
as well. Best way to avoid spurious failures due to lack of a local CUDA / ROCm installation. Maybe in the future LLVM based offloading won't depend on so much external stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added!
Created using spr 1.3.4
@@ -0,0 +1,5 @@ | |||
/// Some target-specific options are ignored for GPU, so %clang exits with code 0. | |||
// DEFINE: %{check} = %clang -### -c -mcmodel=medium |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, what exactly are we checking here? With
-###
CC1 sub-compilations do not run and, I think, that's where unsupported options were triggering warning/errors.
We're still only running the top-level driver. Weren't errors reported by cc1 compilation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably depends on the option we're testing. We could do both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this particular case, the changes we test (and the error messages) were originating in the driver, so we're OK with -###
.
That said, we do have other options that we do need to suppress/ignore and some of that does happen on cc1 level.
We'll eventually need to add the tests for those here and then enable actual compilation, but it should not hold this patch back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general the error reporting is done in the driver and cc1 allows and ignores options that may be specific to other targets.
That said, we do have other options that we do need to suppress/ignore and some of that does happen on cc1 level.
Perhaps we should figure out them and check whether they should be moved to the driver
We started seeing a test failure in our Clang toolchain builders for Mac:
|
Maybe need to specify |
Fix missing test coverage after #70740 #70760
When compiling for CUDA/HIP, the driver creates a cc1 job to compile for amdgcn/nvptx triple using most options.
Certain target-specific options should be ignored, not lead to an error (
err_drv_unsupported_opt_for_target
).