Skip to content

Conversation

@sarnex
Copy link
Member

@sarnex sarnex commented Feb 12, 2025

I'm adding support for SPIR-V, so let's consolidate these checks.

bool isGPUDistribute =
CGM.getLangOpts().OpenMPIsTargetDevice &&
(CGM.getTriple().isAMDGCN() || CGM.getTriple().isNVPTX());
bool isGPUDistribute = CGM.getLangOpts().OpenMPIsTargetDevice &&
Copy link
Member Author

@sarnex sarnex Feb 12, 2025

Choose a reason for hiding this comment

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

here (and a few other places) we were checking for AMDGCN, but the new code checks for AMDGPU.

im not sure if theyre basically interchangeable in offloading code, or if i need to be careful, or if the existing code is wrong and we should be using one or the other everywhere

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@arsenm arsenm Feb 13, 2025

Choose a reason for hiding this comment

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

tThey are functionally equivalent given that r600 is essentially dead and doesn't support any offload languages. But probably should stick to amdgpu

Copy link
Contributor

Choose a reason for hiding this comment

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

Think we should do a sed s/isAMDGCN()/isAMDGPU()/g at some point?

Copy link
Contributor

Choose a reason for hiding this comment

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

probably

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, so new code seems fine then

@sarnex sarnex marked this pull request as ready for review February 12, 2025 22:01
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang:codegen IR generation bugs: mangling, exceptions, etc. compiler-rt:sanitizer llvm:transforms clang:openmp OpenMP related changes to Clang labels Feb 12, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 12, 2025

@llvm/pr-subscribers-flang-fir-hlfir
@llvm/pr-subscribers-flang-openmp
@llvm/pr-subscribers-pgo
@llvm/pr-subscribers-flang-driver
@llvm/pr-subscribers-clang
@llvm/pr-subscribers-clang-driver
@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Nick Sarnie (sarnex)

Changes

I'm adding support for SPIR-V, so let's consolidate these checks.


Full diff: https://github.com/llvm/llvm-project/pull/126956.diff

11 Files Affected:

  • (modified) clang/lib/CodeGen/CGOpenMPRuntime.cpp (+3-4)
  • (modified) clang/lib/CodeGen/CodeGenModule.cpp (+1-1)
  • (modified) clang/lib/CodeGen/CodeGenModule.h (+1-2)
  • (modified) clang/lib/Driver/Driver.cpp (+3-4)
  • (modified) clang/lib/Driver/ToolChains/Clang.cpp (+2-4)
  • (modified) clang/lib/Driver/ToolChains/CommonArgs.cpp (+2-2)
  • (modified) clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp (+1-1)
  • (modified) llvm/include/llvm/TargetParser/Triple.h (+5)
  • (modified) llvm/include/llvm/Transforms/IPO/Attributor.h (+1-3)
  • (modified) llvm/lib/Transforms/IPO/Attributor.cpp (+2-2)
  • (modified) llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp (+1-1)
diff --git a/clang/lib/CodeGen/CGOpenMPRuntime.cpp b/clang/lib/CodeGen/CGOpenMPRuntime.cpp
index b679d63874b3b..32cba453b3d10 100644
--- a/clang/lib/CodeGen/CGOpenMPRuntime.cpp
+++ b/clang/lib/CodeGen/CGOpenMPRuntime.cpp
@@ -2624,9 +2624,8 @@ void CGOpenMPRuntime::emitDistributeStaticInit(
       emitUpdateLocation(CGF, Loc, OMP_IDENT_WORK_DISTRIBUTE);
   llvm::Value *ThreadId = getThreadID(CGF, Loc);
   llvm::FunctionCallee StaticInitFunction;
-  bool isGPUDistribute =
-      CGM.getLangOpts().OpenMPIsTargetDevice &&
-      (CGM.getTriple().isAMDGCN() || CGM.getTriple().isNVPTX());
+  bool isGPUDistribute = CGM.getLangOpts().OpenMPIsTargetDevice &&
+                         CGM.getTriple().isOffloadingTargetGPU();
   StaticInitFunction = OMPBuilder.createForStaticInitFunction(
       Values.IVSize, Values.IVSigned, isGPUDistribute);
 
@@ -2656,7 +2655,7 @@ void CGOpenMPRuntime::emitForStaticFinish(CodeGenFunction &CGF,
   auto DL = ApplyDebugLocation::CreateDefaultArtificial(CGF, Loc);
   if (isOpenMPDistributeDirective(DKind) &&
       CGM.getLangOpts().OpenMPIsTargetDevice &&
-      (CGM.getTriple().isAMDGCN() || CGM.getTriple().isNVPTX()))
+      CGM.getTriple().isOffloadingTargetGPU())
     CGF.EmitRuntimeCall(
         OMPBuilder.getOrCreateRuntimeFunction(
             CGM.getModule(), OMPRTL___kmpc_distribute_static_fini),
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index 7924c32fcf633..99c8f6e547e87 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -842,7 +842,7 @@ static void setVisibilityFromDLLStorageClass(const clang::LangOptions &LO,
 static bool isStackProtectorOn(const LangOptions &LangOpts,
                                const llvm::Triple &Triple,
                                clang::LangOptions::StackProtectorMode Mode) {
-  if (Triple.isAMDGPU() || Triple.isNVPTX())
+  if (Triple.isOffloadingTargetGPU())
     return false;
   return LangOpts.getStackProtector() == Mode;
 }
diff --git a/clang/lib/CodeGen/CodeGenModule.h b/clang/lib/CodeGen/CodeGenModule.h
index c6f6fd5b9a7bd..f197c420f6d44 100644
--- a/clang/lib/CodeGen/CodeGenModule.h
+++ b/clang/lib/CodeGen/CodeGenModule.h
@@ -1067,8 +1067,7 @@ class CodeGenModule : public CodeGenTypeCache {
   bool shouldEmitRTTI(bool ForEH = false) {
     return (ForEH || getLangOpts().RTTI) && !getLangOpts().CUDAIsDevice &&
            !(getLangOpts().OpenMP && getLangOpts().OpenMPIsTargetDevice &&
-             (getTriple().isNVPTX() || getTriple().isAMDGPU() ||
-              getTriple().isSPIRV()));
+             getTriple().isOffloadingTargetGPU());
   }
 
   /// Get the address of the RTTI descriptor for the given type.
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index 50941d2aaa429..b1847322add16 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -3351,10 +3351,9 @@ class OffloadingActionBuilder final {
 
       const ToolChain *HostTC = C.getSingleOffloadToolChain<Action::OFK_Host>();
       assert(HostTC && "No toolchain for host compilation.");
-      if (HostTC->getTriple().isNVPTX() ||
-          HostTC->getTriple().getArch() == llvm::Triple::amdgcn) {
-        // We do not support targeting NVPTX/AMDGCN for host compilation. Throw
-        // an error and abort pipeline construction early so we don't trip
+      if (HostTC->getTriple().isOffloadingTargetGPU()) {
+        // We do not support targeting offloading GPUs for host compilation.
+        // Throw an error and abort pipeline construction early so we don't trip
         // asserts that assume device-side compilation.
         C.getDriver().Diag(diag::err_drv_cuda_host_arch)
             << HostTC->getTriple().getArchName();
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 5deafa2ad0f4a..b25a15338d9ca 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -1133,8 +1133,7 @@ void Clang::AddPreprocessingOptions(Compilation &C, const JobAction &JA,
   if (JA.isDeviceOffloading(Action::OFK_OpenMP) &&
       !Args.hasArg(options::OPT_nostdinc) &&
       !Args.hasArg(options::OPT_nogpuinc) &&
-      (getToolChain().getTriple().isNVPTX() ||
-       getToolChain().getTriple().isAMDGCN())) {
+      getToolChain().getTriple().isOffloadingTargetGPU()) {
     if (!Args.hasArg(options::OPT_nobuiltininc)) {
       // Add openmp_wrappers/* to our system include path.  This lets us wrap
       // standard library headers.
@@ -1321,8 +1320,7 @@ void Clang::AddPreprocessingOptions(Compilation &C, const JobAction &JA,
     // Without an offloading language we will include these headers directly.
     // Offloading languages will instead only use the declarations stored in
     // the resource directory at clang/lib/Headers/llvm_libc_wrappers.
-    if ((getToolChain().getTriple().isNVPTX() ||
-         getToolChain().getTriple().isAMDGCN()) &&
+    if (getToolChain().getTriple().isOffloadingTargetGPU() &&
         C.getActiveOffloadKinds() == Action::OFK_None) {
       SmallString<128> P(llvm::sys::path::parent_path(D.Dir));
       llvm::sys::path::append(P, "include");
diff --git a/clang/lib/Driver/ToolChains/CommonArgs.cpp b/clang/lib/Driver/ToolChains/CommonArgs.cpp
index 9a4d3f55c911c..b73cd86579e20 100644
--- a/clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ b/clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -2973,8 +2973,8 @@ void tools::addMCModel(const Driver &D, const llvm::opt::ArgList &Args,
     } else if (Triple.getArch() == llvm::Triple::x86_64) {
       Ok = llvm::is_contained({"small", "kernel", "medium", "large", "tiny"},
                               CM);
-    } else if (Triple.isNVPTX() || Triple.isAMDGPU() || Triple.isSPIRV()) {
-      // NVPTX/AMDGPU/SPIRV does not care about the code model and will accept
+    } else if (Triple.isOffloadingTargetGPU()) {
+      // Offloading GPU targets do not care about the code model and will accept
       // whatever works for the host.
       Ok = true;
     } else if (Triple.isSPARC64()) {
diff --git a/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp b/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
index aa43b2f5f2a1b..99e2c99759a94 100644
--- a/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
+++ b/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
@@ -512,7 +512,7 @@ Expected<StringRef> clang(ArrayRef<StringRef> InputFiles, const ArgList &Args) {
     CmdArgs.push_back(InputFile);
 
   // If this is CPU offloading we copy the input libraries.
-  if (!Triple.isAMDGPU() && !Triple.isNVPTX() && !Triple.isSPIRV()) {
+  if (!Triple.isOffloadingTargetGPU()) {
     CmdArgs.push_back("-Wl,-Bsymbolic");
     CmdArgs.push_back("-shared");
     ArgStringList LinkerArgs;
diff --git a/llvm/include/llvm/TargetParser/Triple.h b/llvm/include/llvm/TargetParser/Triple.h
index 09c0d223d9b4d..0885d2a44245a 100644
--- a/llvm/include/llvm/TargetParser/Triple.h
+++ b/llvm/include/llvm/TargetParser/Triple.h
@@ -1109,6 +1109,11 @@ class Triple {
            Env == llvm::Triple::EABIHF;
   }
 
+  /// Tests if the target represents a GPU which can be offloaded to.
+  bool isOffloadingTargetGPU() const {
+    return isAMDGPU() || isNVPTX() || isSPIRV();
+  }
+
   /// Tests whether the target supports comdat
   bool supportsCOMDAT() const {
     return !(isOSBinFormatMachO() || isOSBinFormatXCOFF() ||
diff --git a/llvm/include/llvm/Transforms/IPO/Attributor.h b/llvm/include/llvm/Transforms/IPO/Attributor.h
index 995ee54a73ce4..5c2c707e24a14 100644
--- a/llvm/include/llvm/Transforms/IPO/Attributor.h
+++ b/llvm/include/llvm/Transforms/IPO/Attributor.h
@@ -1338,9 +1338,7 @@ struct InformationCache {
   bool stackIsAccessibleByOtherThreads() { return !targetIsGPU(); }
 
   /// Return true if the target is a GPU.
-  bool targetIsGPU() {
-    return TargetTriple.isAMDGPU() || TargetTriple.isNVPTX();
-  }
+  bool targetIsGPU() { return TargetTriple.isOffloadingTargetGPU(); }
 
   /// Return all functions that might be called indirectly, only valid for
   /// closed world modules (see isClosedWorldModule).
diff --git a/llvm/lib/Transforms/IPO/Attributor.cpp b/llvm/lib/Transforms/IPO/Attributor.cpp
index a1e1a51b201b0..ba9148d2aedc7 100644
--- a/llvm/lib/Transforms/IPO/Attributor.cpp
+++ b/llvm/lib/Transforms/IPO/Attributor.cpp
@@ -199,7 +199,7 @@ ChangeStatus &llvm::operator&=(ChangeStatus &L, ChangeStatus R) {
 
 bool AA::isGPU(const Module &M) {
   Triple T(M.getTargetTriple());
-  return T.isAMDGPU() || T.isNVPTX();
+  return T.isOffloadingTargetGPU();
 }
 
 bool AA::isNoSyncInst(Attributor &A, const Instruction &I,
@@ -3296,7 +3296,7 @@ InformationCache::getIndirectlyCallableFunctions(Attributor &A) const {
 }
 
 std::optional<unsigned> InformationCache::getFlatAddressSpace() const {
-  if (TargetTriple.isAMDGPU() || TargetTriple.isNVPTX())
+  if (TargetTriple.isOffloadingTargetGPU())
     return 0;
   return std::nullopt;
 }
diff --git a/llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp b/llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp
index 1759b95ddbc30..85cfff005cb53 100644
--- a/llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp
+++ b/llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp
@@ -133,7 +133,7 @@ class SanitizerBinaryMetadata {
         VersionStr(utostr(getVersion())), IRB(M.getContext()) {
     // FIXME: Make it work with other formats.
     assert(TargetTriple.isOSBinFormatELF() && "ELF only");
-    assert(!(TargetTriple.isNVPTX() || TargetTriple.isAMDGPU()) &&
+    assert(!(TargetTriple.isOffloadingTargetGPU()) &&
            "Device targets are not supported");
   }
 

@llvmbot
Copy link
Member

llvmbot commented Feb 12, 2025

@llvm/pr-subscribers-llvm-transforms

Author: Nick Sarnie (sarnex)

Changes

I'm adding support for SPIR-V, so let's consolidate these checks.


Full diff: https://github.com/llvm/llvm-project/pull/126956.diff

11 Files Affected:

  • (modified) clang/lib/CodeGen/CGOpenMPRuntime.cpp (+3-4)
  • (modified) clang/lib/CodeGen/CodeGenModule.cpp (+1-1)
  • (modified) clang/lib/CodeGen/CodeGenModule.h (+1-2)
  • (modified) clang/lib/Driver/Driver.cpp (+3-4)
  • (modified) clang/lib/Driver/ToolChains/Clang.cpp (+2-4)
  • (modified) clang/lib/Driver/ToolChains/CommonArgs.cpp (+2-2)
  • (modified) clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp (+1-1)
  • (modified) llvm/include/llvm/TargetParser/Triple.h (+5)
  • (modified) llvm/include/llvm/Transforms/IPO/Attributor.h (+1-3)
  • (modified) llvm/lib/Transforms/IPO/Attributor.cpp (+2-2)
  • (modified) llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp (+1-1)
diff --git a/clang/lib/CodeGen/CGOpenMPRuntime.cpp b/clang/lib/CodeGen/CGOpenMPRuntime.cpp
index b679d63874b3b..32cba453b3d10 100644
--- a/clang/lib/CodeGen/CGOpenMPRuntime.cpp
+++ b/clang/lib/CodeGen/CGOpenMPRuntime.cpp
@@ -2624,9 +2624,8 @@ void CGOpenMPRuntime::emitDistributeStaticInit(
       emitUpdateLocation(CGF, Loc, OMP_IDENT_WORK_DISTRIBUTE);
   llvm::Value *ThreadId = getThreadID(CGF, Loc);
   llvm::FunctionCallee StaticInitFunction;
-  bool isGPUDistribute =
-      CGM.getLangOpts().OpenMPIsTargetDevice &&
-      (CGM.getTriple().isAMDGCN() || CGM.getTriple().isNVPTX());
+  bool isGPUDistribute = CGM.getLangOpts().OpenMPIsTargetDevice &&
+                         CGM.getTriple().isOffloadingTargetGPU();
   StaticInitFunction = OMPBuilder.createForStaticInitFunction(
       Values.IVSize, Values.IVSigned, isGPUDistribute);
 
@@ -2656,7 +2655,7 @@ void CGOpenMPRuntime::emitForStaticFinish(CodeGenFunction &CGF,
   auto DL = ApplyDebugLocation::CreateDefaultArtificial(CGF, Loc);
   if (isOpenMPDistributeDirective(DKind) &&
       CGM.getLangOpts().OpenMPIsTargetDevice &&
-      (CGM.getTriple().isAMDGCN() || CGM.getTriple().isNVPTX()))
+      CGM.getTriple().isOffloadingTargetGPU())
     CGF.EmitRuntimeCall(
         OMPBuilder.getOrCreateRuntimeFunction(
             CGM.getModule(), OMPRTL___kmpc_distribute_static_fini),
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index 7924c32fcf633..99c8f6e547e87 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -842,7 +842,7 @@ static void setVisibilityFromDLLStorageClass(const clang::LangOptions &LO,
 static bool isStackProtectorOn(const LangOptions &LangOpts,
                                const llvm::Triple &Triple,
                                clang::LangOptions::StackProtectorMode Mode) {
-  if (Triple.isAMDGPU() || Triple.isNVPTX())
+  if (Triple.isOffloadingTargetGPU())
     return false;
   return LangOpts.getStackProtector() == Mode;
 }
diff --git a/clang/lib/CodeGen/CodeGenModule.h b/clang/lib/CodeGen/CodeGenModule.h
index c6f6fd5b9a7bd..f197c420f6d44 100644
--- a/clang/lib/CodeGen/CodeGenModule.h
+++ b/clang/lib/CodeGen/CodeGenModule.h
@@ -1067,8 +1067,7 @@ class CodeGenModule : public CodeGenTypeCache {
   bool shouldEmitRTTI(bool ForEH = false) {
     return (ForEH || getLangOpts().RTTI) && !getLangOpts().CUDAIsDevice &&
            !(getLangOpts().OpenMP && getLangOpts().OpenMPIsTargetDevice &&
-             (getTriple().isNVPTX() || getTriple().isAMDGPU() ||
-              getTriple().isSPIRV()));
+             getTriple().isOffloadingTargetGPU());
   }
 
   /// Get the address of the RTTI descriptor for the given type.
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index 50941d2aaa429..b1847322add16 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -3351,10 +3351,9 @@ class OffloadingActionBuilder final {
 
       const ToolChain *HostTC = C.getSingleOffloadToolChain<Action::OFK_Host>();
       assert(HostTC && "No toolchain for host compilation.");
-      if (HostTC->getTriple().isNVPTX() ||
-          HostTC->getTriple().getArch() == llvm::Triple::amdgcn) {
-        // We do not support targeting NVPTX/AMDGCN for host compilation. Throw
-        // an error and abort pipeline construction early so we don't trip
+      if (HostTC->getTriple().isOffloadingTargetGPU()) {
+        // We do not support targeting offloading GPUs for host compilation.
+        // Throw an error and abort pipeline construction early so we don't trip
         // asserts that assume device-side compilation.
         C.getDriver().Diag(diag::err_drv_cuda_host_arch)
             << HostTC->getTriple().getArchName();
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 5deafa2ad0f4a..b25a15338d9ca 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -1133,8 +1133,7 @@ void Clang::AddPreprocessingOptions(Compilation &C, const JobAction &JA,
   if (JA.isDeviceOffloading(Action::OFK_OpenMP) &&
       !Args.hasArg(options::OPT_nostdinc) &&
       !Args.hasArg(options::OPT_nogpuinc) &&
-      (getToolChain().getTriple().isNVPTX() ||
-       getToolChain().getTriple().isAMDGCN())) {
+      getToolChain().getTriple().isOffloadingTargetGPU()) {
     if (!Args.hasArg(options::OPT_nobuiltininc)) {
       // Add openmp_wrappers/* to our system include path.  This lets us wrap
       // standard library headers.
@@ -1321,8 +1320,7 @@ void Clang::AddPreprocessingOptions(Compilation &C, const JobAction &JA,
     // Without an offloading language we will include these headers directly.
     // Offloading languages will instead only use the declarations stored in
     // the resource directory at clang/lib/Headers/llvm_libc_wrappers.
-    if ((getToolChain().getTriple().isNVPTX() ||
-         getToolChain().getTriple().isAMDGCN()) &&
+    if (getToolChain().getTriple().isOffloadingTargetGPU() &&
         C.getActiveOffloadKinds() == Action::OFK_None) {
       SmallString<128> P(llvm::sys::path::parent_path(D.Dir));
       llvm::sys::path::append(P, "include");
diff --git a/clang/lib/Driver/ToolChains/CommonArgs.cpp b/clang/lib/Driver/ToolChains/CommonArgs.cpp
index 9a4d3f55c911c..b73cd86579e20 100644
--- a/clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ b/clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -2973,8 +2973,8 @@ void tools::addMCModel(const Driver &D, const llvm::opt::ArgList &Args,
     } else if (Triple.getArch() == llvm::Triple::x86_64) {
       Ok = llvm::is_contained({"small", "kernel", "medium", "large", "tiny"},
                               CM);
-    } else if (Triple.isNVPTX() || Triple.isAMDGPU() || Triple.isSPIRV()) {
-      // NVPTX/AMDGPU/SPIRV does not care about the code model and will accept
+    } else if (Triple.isOffloadingTargetGPU()) {
+      // Offloading GPU targets do not care about the code model and will accept
       // whatever works for the host.
       Ok = true;
     } else if (Triple.isSPARC64()) {
diff --git a/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp b/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
index aa43b2f5f2a1b..99e2c99759a94 100644
--- a/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
+++ b/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
@@ -512,7 +512,7 @@ Expected<StringRef> clang(ArrayRef<StringRef> InputFiles, const ArgList &Args) {
     CmdArgs.push_back(InputFile);
 
   // If this is CPU offloading we copy the input libraries.
-  if (!Triple.isAMDGPU() && !Triple.isNVPTX() && !Triple.isSPIRV()) {
+  if (!Triple.isOffloadingTargetGPU()) {
     CmdArgs.push_back("-Wl,-Bsymbolic");
     CmdArgs.push_back("-shared");
     ArgStringList LinkerArgs;
diff --git a/llvm/include/llvm/TargetParser/Triple.h b/llvm/include/llvm/TargetParser/Triple.h
index 09c0d223d9b4d..0885d2a44245a 100644
--- a/llvm/include/llvm/TargetParser/Triple.h
+++ b/llvm/include/llvm/TargetParser/Triple.h
@@ -1109,6 +1109,11 @@ class Triple {
            Env == llvm::Triple::EABIHF;
   }
 
+  /// Tests if the target represents a GPU which can be offloaded to.
+  bool isOffloadingTargetGPU() const {
+    return isAMDGPU() || isNVPTX() || isSPIRV();
+  }
+
   /// Tests whether the target supports comdat
   bool supportsCOMDAT() const {
     return !(isOSBinFormatMachO() || isOSBinFormatXCOFF() ||
diff --git a/llvm/include/llvm/Transforms/IPO/Attributor.h b/llvm/include/llvm/Transforms/IPO/Attributor.h
index 995ee54a73ce4..5c2c707e24a14 100644
--- a/llvm/include/llvm/Transforms/IPO/Attributor.h
+++ b/llvm/include/llvm/Transforms/IPO/Attributor.h
@@ -1338,9 +1338,7 @@ struct InformationCache {
   bool stackIsAccessibleByOtherThreads() { return !targetIsGPU(); }
 
   /// Return true if the target is a GPU.
-  bool targetIsGPU() {
-    return TargetTriple.isAMDGPU() || TargetTriple.isNVPTX();
-  }
+  bool targetIsGPU() { return TargetTriple.isOffloadingTargetGPU(); }
 
   /// Return all functions that might be called indirectly, only valid for
   /// closed world modules (see isClosedWorldModule).
diff --git a/llvm/lib/Transforms/IPO/Attributor.cpp b/llvm/lib/Transforms/IPO/Attributor.cpp
index a1e1a51b201b0..ba9148d2aedc7 100644
--- a/llvm/lib/Transforms/IPO/Attributor.cpp
+++ b/llvm/lib/Transforms/IPO/Attributor.cpp
@@ -199,7 +199,7 @@ ChangeStatus &llvm::operator&=(ChangeStatus &L, ChangeStatus R) {
 
 bool AA::isGPU(const Module &M) {
   Triple T(M.getTargetTriple());
-  return T.isAMDGPU() || T.isNVPTX();
+  return T.isOffloadingTargetGPU();
 }
 
 bool AA::isNoSyncInst(Attributor &A, const Instruction &I,
@@ -3296,7 +3296,7 @@ InformationCache::getIndirectlyCallableFunctions(Attributor &A) const {
 }
 
 std::optional<unsigned> InformationCache::getFlatAddressSpace() const {
-  if (TargetTriple.isAMDGPU() || TargetTriple.isNVPTX())
+  if (TargetTriple.isOffloadingTargetGPU())
     return 0;
   return std::nullopt;
 }
diff --git a/llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp b/llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp
index 1759b95ddbc30..85cfff005cb53 100644
--- a/llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp
+++ b/llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp
@@ -133,7 +133,7 @@ class SanitizerBinaryMetadata {
         VersionStr(utostr(getVersion())), IRB(M.getContext()) {
     // FIXME: Make it work with other formats.
     assert(TargetTriple.isOSBinFormatELF() && "ELF only");
-    assert(!(TargetTriple.isNVPTX() || TargetTriple.isAMDGPU()) &&
+    assert(!(TargetTriple.isOffloadingTargetGPU()) &&
            "Device targets are not supported");
   }
 

@sarnex sarnex requested a review from jhuber6 February 12, 2025 22:02
}

/// Tests if the target represents a GPU which can be offloaded to.
bool isOffloadingTargetGPU() const {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it better to use isOffloadingTarget?

Copy link
Member Author

Choose a reason for hiding this comment

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

sure, thats good point. i think doing that will resolve your other comment about SPIR-V being a gpu. ill implement this.

bool targetIsGPU() {
return TargetTriple.isAMDGPU() || TargetTriple.isNVPTX();
}
bool targetIsGPU() { return TargetTriple.isOffloadingTargetGPU(); }
Copy link
Contributor

Choose a reason for hiding this comment

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

This also implies that SPIR-V is a GPU target triple, but is it true?

bool isGPUDistribute =
CGM.getLangOpts().OpenMPIsTargetDevice &&
(CGM.getTriple().isAMDGCN() || CGM.getTriple().isNVPTX());
bool isGPUDistribute = CGM.getLangOpts().OpenMPIsTargetDevice &&
Copy link
Contributor

Choose a reason for hiding this comment

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

@sarnex sarnex changed the title [LLVM][Triple][NFCI] Add function to test for GPU offloading triples [LLVM][Triple][NFCI] Add function to test for offloading triples Feb 13, 2025
@sarnex sarnex requested review from arsenm and shiltian February 14, 2025 15:14
Copy link
Contributor

@shiltian shiltian left a comment

Choose a reason for hiding this comment

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

LGTM but I'd like to get a second stamp on it.

Comment on lines 1113 to 1115
Copy link
Contributor

@jhuber6 jhuber6 Feb 17, 2025

Choose a reason for hiding this comment

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

I don't think this belongs in regular triple handling. It could be a more generic 'isTargetGPU` but I think there were some objections given that SPIR-V can target other things?

My concern is that this is a triple which just handles a single target, while the concept of 'offloading' is only relevant with an auxiliary triple. I think this is better as a helper in Clang, we could possibly even combine all of these isCUDADevice, isOpenMPDevice checks as well which would make this more useful. (Thinking about this since #127439 has another one of these uses.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, was OOO. Will work to address this soon.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for late response.

So for cases like in the linked PR, we want to know if we are offloading by checking the main/aux triple through the ASTContext. Once we are into the middle end that information won't be available as far as I can tell.

Do you have an idea on how we can detect offloading at all phases in the compiler without solely using the triple?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jhuber6 Any recommendations on the above?

Copy link
Contributor

Choose a reason for hiding this comment

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

Offloading is a clang concept, it's usually available through the language info, which should be available all the way to codegen. For example CGM.getLangOpts().OpenMPIsTargetDevice.

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it, makes sense, will do.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe something like the latest commit is closer to what you had in mind?

@llvmbot llvmbot added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label Mar 5, 2025
@sarnex sarnex changed the title [LLVM][Triple][NFCI] Add function to test for offloading triples [clang] Add isOffloadingTarget function to LangOpts Mar 5, 2025
Copy link
Contributor

@jhuber6 jhuber6 left a comment

Choose a reason for hiding this comment

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

Thanks

@sarnex sarnex requested a review from Fznamznon March 5, 2025 21:13
Copy link
Contributor

@Fznamznon Fznamznon left a comment

Choose a reason for hiding this comment

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

I appreciate the generalization, thanks!

Copy link
Contributor

@arsenm arsenm left a comment

Choose a reason for hiding this comment

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

The target isn't part of the language, why is it in LangOpts?

@sarnex
Copy link
Member Author

sarnex commented Mar 6, 2025

The target isn't part of the language, why is it in LangOpts?

If you have a better suggestion I'm all ears, we seem to already have similar stuff in LangOpts such as OMPTargetTriples, OMPHostIRFile, GPUDefaultStream, and CUID.

@sarnex
Copy link
Member Author

sarnex commented Mar 10, 2025

@arsenm Any comments on the above? Thx

@sarnex
Copy link
Member Author

sarnex commented Mar 17, 2025

@arsenm Ping :)

}

Opts.IsOffloadingTarget =
(Opts.OpenMPIsTargetDevice || Opts.SYCLIsDevice || Opts.CUDAIsDevice) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the concern is that this isn't strictly a language option since it depends on the target. This is more like "isGPUOffloadingTarget." We could possibly split these into two. One goes in the TargetInfo and one goes in the LangOpts?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, what would the two flags be? IsOffloading and IsGPU?

Copy link
Contributor

Choose a reason for hiding this comment

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

Presumably, yeah.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, back into the depths.

Signed-off-by: Sarnie, Nick <[email protected]>
@llvmbot llvmbot added flang:driver PGO Profile Guided Optimizations flang Flang issues not falling into any other category labels Mar 21, 2025
Copy link
Contributor

@jhuber6 jhuber6 left a comment

Choose a reason for hiding this comment

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

Seems reasonable so long as we know that SPIR-V is always a GPU. I'll defer to SPIR people.

@sarnex sarnex changed the title [clang] Add isOffloadingTarget function to LangOpts [clang][flang][Triple][llvm] Add isOffload function to LangOpts and isGPU function to Triple Mar 21, 2025
Signed-off-by: Sarnie, Nick <[email protected]>
@sarnex sarnex requested a review from arsenm March 25, 2025 16:19
@sarnex sarnex merged commit 48b7530 into llvm:main Mar 28, 2025
13 checks passed
@hvdijk
Copy link
Contributor

hvdijk commented Apr 10, 2025

Seems reasonable so long as we know that SPIR-V is always a GPU. I'll defer to SPIR people.

It's not. SPIR-V is mostly-device-agnostic and implementations may run it on GPU, on CPU, or on any other device. I'm looking at this while trying to update 94229, I think isGPU() should return false for SPIR-V, and instead there should be checks for SYCL rather than for SPIR-V as non-SPIR-V SYCL targets need the same handling, does that sound reasonable to you?

@arsenm
Copy link
Contributor

arsenm commented Apr 10, 2025

Seems reasonable so long as we know that sound reasonable to you?

No. This is a bikeshedding problem on the isGPU name. SPIRV is functionally a "GPU" target. The abstract future physical device it nay run on is unimportant

@hvdijk
Copy link
Contributor

hvdijk commented Apr 10, 2025

It's not a bikeshedding problem that it doesn't handle other SYCL targets consistently though. In some places there are checks for SYCL, in other places there are checks for SPIR-V.

@arsenm
Copy link
Contributor

arsenm commented Apr 10, 2025

It's not a bikeshedding problem that it doesn't handle other SYCL targets consistently though. In some places there are checks for SYCL, in other places there are checks for SPIR-V.

Usage context wrong is a different problem than whether SPIRV should count as "isGPU"

@hvdijk
Copy link
Contributor

hvdijk commented Apr 10, 2025

I did include "as non-SPIR-V SYCL targets need the same handling" already in my first comment, so not sure why it gets that strong immediate reaction that ignores that part of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clang:codegen IR generation bugs: mangling, exceptions, etc. clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:openmp OpenMP related changes to Clang clang Clang issues not falling into any other category compiler-rt:sanitizer flang:driver flang:fir-hlfir flang:openmp flang Flang issues not falling into any other category llvm:transforms PGO Profile Guided Optimizations

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants