[CUDA][HIP] add option -gpuinc #140106
[CUDA][HIP] add option -gpuinc #140106
Conversation
Currently there is only option -nogpuinc for disabling the default CUDA/HIP wrapper headers. However, there are situations where -nogpuinc needs to be overriden for enabling CUDA/HIP wrapper headers. This patch adds -gpuinc for that purpose. When both exist, the last wins.
llvmbot
added
clang
|
@llvm/pr-subscribers-backend-amdgpu @llvm/pr-subscribers-clang Author: Yaxun (Sam) Liu (yxsamliu) ChangesCurrently there is only option -nogpuinc for disabling the default CUDA/HIP wrapper headers. However, there are situations where -nogpuinc needs to be overriden for enabling CUDA/HIP wrapper headers. This patch Full diff: https://github.com/llvm/llvm-project/pull/140106.diff 6 Files Affected:
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index bd8df8f6a749a..05796e73ba3c2 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -5734,6 +5734,9 @@ def nobuiltininc : Flag<["-"], "nobuiltininc">,
def nogpuinc : Flag<["-"], "nogpuinc">, Group<IncludePath_Group>,
HelpText<"Do not add include paths for CUDA/HIP and"
" do not include the default CUDA/HIP wrapper headers">;
+def gpuinc : Flag<["-"], "gpuinc">, Group<IncludePath_Group>,
+ HelpText<"Add include paths for CUDA/HIP and"
+ " include the default CUDA/HIP wrapper headers">;
def nohipwrapperinc : Flag<["-"], "nohipwrapperinc">, Group<IncludePath_Group>,
HelpText<"Do not include the default HIP wrapper headers and include paths">;
def : Flag<["-"], "nocudainc">, Alias<nogpuinc>;
diff --git a/clang/lib/Driver/ToolChains/AMDGPU.cpp b/clang/lib/Driver/ToolChains/AMDGPU.cpp
index 35ca019795ddc..c6d66d2c6b452 100644
--- a/clang/lib/Driver/ToolChains/AMDGPU.cpp
+++ b/clang/lib/Driver/ToolChains/AMDGPU.cpp
@@ -526,7 +526,7 @@ void RocmInstallationDetector::AddHIPIncludeArgs(const ArgList &DriverArgs,
"hipstdpar_lib.hpp"});
};
- if (DriverArgs.hasArg(options::OPT_nogpuinc)) {
+ if (!DriverArgs.hasFlag(options::OPT_gpuinc, options::OPT_nogpuinc, true)) {
if (HasHipStdPar)
HandleHipStdPar();
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index a08bdba99bfe0..cfd7ed49e0b99 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -1107,7 +1107,7 @@ void Clang::AddPreprocessingOptions(Compilation &C, const JobAction &JA,
// openmp_wrappers folder which contains alternative system headers.
if (JA.isDeviceOffloading(Action::OFK_OpenMP) &&
!Args.hasArg(options::OPT_nostdinc) &&
- !Args.hasArg(options::OPT_nogpuinc) &&
+ Args.hasFlag(options::OPT_gpuinc, options::OPT_nogpuinc, true) &&
getToolChain().getTriple().isGPU()) {
if (!Args.hasArg(options::OPT_nobuiltininc)) {
// Add openmp_wrappers/* to our system include path. This lets us wrap
@@ -1290,7 +1290,7 @@ void Clang::AddPreprocessingOptions(Compilation &C, const JobAction &JA,
// TODO: This should be moved to `AddClangSystemIncludeArgs` by passing the
// OffloadKind as an argument.
if (!Args.hasArg(options::OPT_nostdinc) &&
- !Args.hasArg(options::OPT_nogpuinc) &&
+ Args.hasFlag(options::OPT_gpuinc, options::OPT_nogpuinc, true) &&
!Args.hasArg(options::OPT_nobuiltininc)) {
// Without an offloading language we will include these headers directly.
// Offloading languages will instead only use the declarations stored in
diff --git a/clang/lib/Driver/ToolChains/Cuda.cpp b/clang/lib/Driver/ToolChains/Cuda.cpp
index 06b0b0913d24e..0a34481ec7060 100644
--- a/clang/lib/Driver/ToolChains/Cuda.cpp
+++ b/clang/lib/Driver/ToolChains/Cuda.cpp
@@ -305,7 +305,7 @@ void CudaInstallationDetector::AddCudaIncludeArgs(
CC1Args.push_back(DriverArgs.MakeArgString(P));
}
- if (DriverArgs.hasArg(options::OPT_nogpuinc))
+ if (!DriverArgs.hasFlag(options::OPT_gpuinc, options::OPT_nogpuinc, true))
return;
if (!isValid()) {
@@ -931,7 +931,7 @@ llvm::DenormalMode CudaToolChain::getDefaultDenormalModeForType(
void CudaToolChain::AddCudaIncludeArgs(const ArgList &DriverArgs,
ArgStringList &CC1Args) const {
// Check our CUDA version if we're going to include the CUDA headers.
- if (!DriverArgs.hasArg(options::OPT_nogpuinc) &&
+ if (DriverArgs.hasFlag(options::OPT_gpuinc, options::OPT_nogpuinc, true) &&
!DriverArgs.hasArg(options::OPT_no_cuda_version_check)) {
StringRef Arch = DriverArgs.getLastArgValue(options::OPT_march_EQ);
assert(!Arch.empty() && "Must have an explicit GPU arch.");
@@ -1004,7 +1004,8 @@ void CudaToolChain::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
ArgStringList &CC1Args) const {
HostTC.AddClangSystemIncludeArgs(DriverArgs, CC1Args);
- if (!DriverArgs.hasArg(options::OPT_nogpuinc) && CudaInstallation.isValid())
+ if (DriverArgs.hasFlag(options::OPT_gpuinc, options::OPT_nogpuinc, true) &&
+ CudaInstallation.isValid())
CC1Args.append(
{"-internal-isystem",
DriverArgs.MakeArgString(CudaInstallation.getIncludePath())});
diff --git a/clang/lib/Driver/ToolChains/HIPSPV.cpp b/clang/lib/Driver/ToolChains/HIPSPV.cpp
index bbde5c047c3f9..c6b8e35a7c61d 100644
--- a/clang/lib/Driver/ToolChains/HIPSPV.cpp
+++ b/clang/lib/Driver/ToolChains/HIPSPV.cpp
@@ -188,7 +188,7 @@ void HIPSPVToolChain::AddIAMCUIncludeArgs(const ArgList &Args,
void HIPSPVToolChain::AddHIPIncludeArgs(const ArgList &DriverArgs,
ArgStringList &CC1Args) const {
- if (DriverArgs.hasArg(options::OPT_nogpuinc))
+ if (!DriverArgs.hasFlag(options::OPT_gpuinc, options::OPT_nogpuinc, true))
return;
StringRef hipPath = DriverArgs.getLastArgValue(options::OPT_hip_path_EQ);
diff --git a/clang/test/Driver/hip-include-path.hip b/clang/test/Driver/hip-include-path.hip
index 5eeee2f5ce0d5..cb2d5b0bed173 100644
--- a/clang/test/Driver/hip-include-path.hip
+++ b/clang/test/Driver/hip-include-path.hip
@@ -12,6 +12,10 @@
// RUN: -std=c++11 --rocm-path=%S/Inputs/rocm -nogpuinc -nogpulib %s 2>&1 \
// RUN: | FileCheck -check-prefixes=COMMON,CLANG,NOHIP %s
+// RUN: %clang -c -### --target=x86_64-unknown-linux-gnu --cuda-gpu-arch=gfx900 \
+// RUN: -std=c++11 --rocm-path=%S/Inputs/rocm -nogpuinc -nogpulib -gpuinc %s 2>&1 \
+// RUN: | FileCheck -check-prefixes=COMMON,CLANG,HIP %s
+
// COMMON-LABEL: "{{[^"]*}}clang{{[^"]*}}" "-cc1"
// CLANG-SAME: "-internal-isystem" "{{[^"]*}}/lib{{[^"]*}}/clang/{{[^"]*}}/include/cuda_wrappers"
// NOCLANG-NOT: "{{[^"]*}}/lib{{[^"]*}}/clang/{{[^"]*}}/include/cuda_wrappers"
|
|
@llvm/pr-subscribers-clang-driver Author: Yaxun (Sam) Liu (yxsamliu) ChangesCurrently there is only option -nogpuinc for disabling the default CUDA/HIP wrapper headers. However, there are situations where -nogpuinc needs to be overriden for enabling CUDA/HIP wrapper headers. This patch Full diff: https://github.com/llvm/llvm-project/pull/140106.diff 6 Files Affected:
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index bd8df8f6a749a..05796e73ba3c2 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -5734,6 +5734,9 @@ def nobuiltininc : Flag<["-"], "nobuiltininc">,
def nogpuinc : Flag<["-"], "nogpuinc">, Group<IncludePath_Group>,
HelpText<"Do not add include paths for CUDA/HIP and"
" do not include the default CUDA/HIP wrapper headers">;
+def gpuinc : Flag<["-"], "gpuinc">, Group<IncludePath_Group>,
+ HelpText<"Add include paths for CUDA/HIP and"
+ " include the default CUDA/HIP wrapper headers">;
def nohipwrapperinc : Flag<["-"], "nohipwrapperinc">, Group<IncludePath_Group>,
HelpText<"Do not include the default HIP wrapper headers and include paths">;
def : Flag<["-"], "nocudainc">, Alias<nogpuinc>;
diff --git a/clang/lib/Driver/ToolChains/AMDGPU.cpp b/clang/lib/Driver/ToolChains/AMDGPU.cpp
index 35ca019795ddc..c6d66d2c6b452 100644
--- a/clang/lib/Driver/ToolChains/AMDGPU.cpp
+++ b/clang/lib/Driver/ToolChains/AMDGPU.cpp
@@ -526,7 +526,7 @@ void RocmInstallationDetector::AddHIPIncludeArgs(const ArgList &DriverArgs,
"hipstdpar_lib.hpp"});
};
- if (DriverArgs.hasArg(options::OPT_nogpuinc)) {
+ if (!DriverArgs.hasFlag(options::OPT_gpuinc, options::OPT_nogpuinc, true)) {
if (HasHipStdPar)
HandleHipStdPar();
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index a08bdba99bfe0..cfd7ed49e0b99 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -1107,7 +1107,7 @@ void Clang::AddPreprocessingOptions(Compilation &C, const JobAction &JA,
// openmp_wrappers folder which contains alternative system headers.
if (JA.isDeviceOffloading(Action::OFK_OpenMP) &&
!Args.hasArg(options::OPT_nostdinc) &&
- !Args.hasArg(options::OPT_nogpuinc) &&
+ Args.hasFlag(options::OPT_gpuinc, options::OPT_nogpuinc, true) &&
getToolChain().getTriple().isGPU()) {
if (!Args.hasArg(options::OPT_nobuiltininc)) {
// Add openmp_wrappers/* to our system include path. This lets us wrap
@@ -1290,7 +1290,7 @@ void Clang::AddPreprocessingOptions(Compilation &C, const JobAction &JA,
// TODO: This should be moved to `AddClangSystemIncludeArgs` by passing the
// OffloadKind as an argument.
if (!Args.hasArg(options::OPT_nostdinc) &&
- !Args.hasArg(options::OPT_nogpuinc) &&
+ Args.hasFlag(options::OPT_gpuinc, options::OPT_nogpuinc, true) &&
!Args.hasArg(options::OPT_nobuiltininc)) {
// Without an offloading language we will include these headers directly.
// Offloading languages will instead only use the declarations stored in
diff --git a/clang/lib/Driver/ToolChains/Cuda.cpp b/clang/lib/Driver/ToolChains/Cuda.cpp
index 06b0b0913d24e..0a34481ec7060 100644
--- a/clang/lib/Driver/ToolChains/Cuda.cpp
+++ b/clang/lib/Driver/ToolChains/Cuda.cpp
@@ -305,7 +305,7 @@ void CudaInstallationDetector::AddCudaIncludeArgs(
CC1Args.push_back(DriverArgs.MakeArgString(P));
}
- if (DriverArgs.hasArg(options::OPT_nogpuinc))
+ if (!DriverArgs.hasFlag(options::OPT_gpuinc, options::OPT_nogpuinc, true))
return;
if (!isValid()) {
@@ -931,7 +931,7 @@ llvm::DenormalMode CudaToolChain::getDefaultDenormalModeForType(
void CudaToolChain::AddCudaIncludeArgs(const ArgList &DriverArgs,
ArgStringList &CC1Args) const {
// Check our CUDA version if we're going to include the CUDA headers.
- if (!DriverArgs.hasArg(options::OPT_nogpuinc) &&
+ if (DriverArgs.hasFlag(options::OPT_gpuinc, options::OPT_nogpuinc, true) &&
!DriverArgs.hasArg(options::OPT_no_cuda_version_check)) {
StringRef Arch = DriverArgs.getLastArgValue(options::OPT_march_EQ);
assert(!Arch.empty() && "Must have an explicit GPU arch.");
@@ -1004,7 +1004,8 @@ void CudaToolChain::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
ArgStringList &CC1Args) const {
HostTC.AddClangSystemIncludeArgs(DriverArgs, CC1Args);
- if (!DriverArgs.hasArg(options::OPT_nogpuinc) && CudaInstallation.isValid())
+ if (DriverArgs.hasFlag(options::OPT_gpuinc, options::OPT_nogpuinc, true) &&
+ CudaInstallation.isValid())
CC1Args.append(
{"-internal-isystem",
DriverArgs.MakeArgString(CudaInstallation.getIncludePath())});
diff --git a/clang/lib/Driver/ToolChains/HIPSPV.cpp b/clang/lib/Driver/ToolChains/HIPSPV.cpp
index bbde5c047c3f9..c6b8e35a7c61d 100644
--- a/clang/lib/Driver/ToolChains/HIPSPV.cpp
+++ b/clang/lib/Driver/ToolChains/HIPSPV.cpp
@@ -188,7 +188,7 @@ void HIPSPVToolChain::AddIAMCUIncludeArgs(const ArgList &Args,
void HIPSPVToolChain::AddHIPIncludeArgs(const ArgList &DriverArgs,
ArgStringList &CC1Args) const {
- if (DriverArgs.hasArg(options::OPT_nogpuinc))
+ if (!DriverArgs.hasFlag(options::OPT_gpuinc, options::OPT_nogpuinc, true))
return;
StringRef hipPath = DriverArgs.getLastArgValue(options::OPT_hip_path_EQ);
diff --git a/clang/test/Driver/hip-include-path.hip b/clang/test/Driver/hip-include-path.hip
index 5eeee2f5ce0d5..cb2d5b0bed173 100644
--- a/clang/test/Driver/hip-include-path.hip
+++ b/clang/test/Driver/hip-include-path.hip
@@ -12,6 +12,10 @@
// RUN: -std=c++11 --rocm-path=%S/Inputs/rocm -nogpuinc -nogpulib %s 2>&1 \
// RUN: | FileCheck -check-prefixes=COMMON,CLANG,NOHIP %s
+// RUN: %clang -c -### --target=x86_64-unknown-linux-gnu --cuda-gpu-arch=gfx900 \
+// RUN: -std=c++11 --rocm-path=%S/Inputs/rocm -nogpuinc -nogpulib -gpuinc %s 2>&1 \
+// RUN: | FileCheck -check-prefixes=COMMON,CLANG,HIP %s
+
// COMMON-LABEL: "{{[^"]*}}clang{{[^"]*}}" "-cc1"
// CLANG-SAME: "-internal-isystem" "{{[^"]*}}/lib{{[^"]*}}/clang/{{[^"]*}}/include/cuda_wrappers"
// NOCLANG-NOT: "{{[^"]*}}/lib{{[^"]*}}/clang/{{[^"]*}}/include/cuda_wrappers"
|
|
Hmm, in what cases is |
comgr is the JIT compiler for HIP on ROCm. comgr uses -nogpuinc by default. However, some users of comgr need to override that so that comgr enables the wrapper headers. We cannot simply let comgr stop using -nogpuinc by default since it will break existing comgr users. Then we need to add -gpuinc to override that. |
I guess it's too bothersome to manually add |
The user app of comgr does not know about the clang wrapper path and name, which is internals of clang and may change. |
Is it really easier to patch clang over comgr? |
Can we make |
It is possible to patch comgr, but I think in general we better to have pairs of flag to be able to override them. The same request popped up before for some reasons. |
but there is other comgr user expecting comgr to have -nogpuinc by default. changing that will cause regressions. |
If |
As I said, I think this is just one case that we need an option to override -nogpuinc. changes are we will need it in other situations. |
There was a problem hiding this comment.
Being able to override a flag is a good thing to have, IMO. There are builds where the owner of the leaf targets do not have much control over which options are set by the "default" compilation, so they need to rely on being able to override preceding options.
The fact that we didn't need it till now for this particular option, and that we may be able to work around this particular use case does not negate conceptual usefulness, but makes it a "nice to have" thing, rather than a "must fix that".
While we're at that, perhaps we should consider leaving those legacy-style flags as is, and implement new-style overridable --[no-]offload-{inc,lib} ?
| def nogpuinc : Flag<["-"], "nogpuinc">, Group<IncludePath_Group>, | ||
| HelpText<"Do not add include paths for CUDA/HIP and" | ||
| " do not include the default CUDA/HIP wrapper headers">; | ||
| def gpuinc : Flag<["-"], "gpuinc">, Group<IncludePath_Group>, | ||
| HelpText<"Add include paths for CUDA/HIP and" | ||
| " include the default CUDA/HIP wrapper headers">; |
There was a problem hiding this comment.
Can we consolidate them into s single boolean flag?
| @@ -5734,6 +5734,9 @@ def nobuiltininc : Flag<["-"], "nobuiltininc">, | |||
| def nogpuinc : Flag<["-"], "nogpuinc">, Group<IncludePath_Group>, | |||
| HelpText<"Do not add include paths for CUDA/HIP and" | |||
| " do not include the default CUDA/HIP wrapper headers">; | |||
| def gpuinc : Flag<["-"], "gpuinc">, Group<IncludePath_Group>, | |||
| HelpText<"Add include paths for CUDA/HIP and" | |||
| " include the default CUDA/HIP wrapper headers">; | |||
| def nohipwrapperinc : Flag<["-"], "nohipwrapperinc">, Group<IncludePath_Group>, | |||
| HelpText<"Do not include the default HIP wrapper headers and include paths">; | |||
| def : Flag<["-"], "nocudainc">, Alias<nogpuinc>; | |||
There was a problem hiding this comment.
we should also have an alias for cudainc, too.
|
I thought we already added a generic -stdinc/nostdinc for this |
there is only -nostdinc but there is no -stdinc |
Yes I think this is an opportunity to make this change so that these two options are more composable |
Currently there is only option -nogpuinc for disabling the default CUDA/HIP wrapper headers. However, there are situations where -nogpuinc needs to be overriden for enabling CUDA/HIP wrapper headers. This patch
adds -gpuinc for that purpose. When both exist, the last wins.