Skip to content

[MLIR][NVVM] Reduce the scope of the LLVM_HAS_NVPTX_TARGET guard #97349

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

Merged
merged 1 commit into from
Jul 2, 2024

Conversation

joker-eph
Copy link
Collaborator

Most of the code here does not depend on the NVPTX target. In particular the simple offload can just emit LLVM IR and we can use this without the NVVM backend being built, which can be useful for a frontend that just need to serialize the IR and leave it up to the runtime to JIT further.

@llvmbot
Copy link
Member

llvmbot commented Jul 1, 2024

@llvm/pr-subscribers-mlir
@llvm/pr-subscribers-mlir-gpu

@llvm/pr-subscribers-mlir-llvm

Author: Mehdi Amini (joker-eph)

Changes

Most of the code here does not depend on the NVPTX target. In particular the simple offload can just emit LLVM IR and we can use this without the NVVM backend being built, which can be useful for a frontend that just need to serialize the IR and leave it up to the runtime to JIT further.


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

2 Files Affected:

  • (modified) mlir/lib/Dialect/GPU/Transforms/ModuleToBinary.cpp (-2)
  • (modified) mlir/lib/Target/LLVM/NVVM/Target.cpp (+6-8)
diff --git a/mlir/lib/Dialect/GPU/Transforms/ModuleToBinary.cpp b/mlir/lib/Dialect/GPU/Transforms/ModuleToBinary.cpp
index 1e7596e8cc4af..62b438656f03f 100644
--- a/mlir/lib/Dialect/GPU/Transforms/ModuleToBinary.cpp
+++ b/mlir/lib/Dialect/GPU/Transforms/ModuleToBinary.cpp
@@ -48,9 +48,7 @@ void GpuModuleToBinaryPass::getDependentDialects(
   // Register all GPU related translations.
   registry.insert<gpu::GPUDialect>();
   registry.insert<LLVM::LLVMDialect>();
-#if LLVM_HAS_NVPTX_TARGET
   registry.insert<NVVM::NVVMDialect>();
-#endif
 #if MLIR_ENABLE_ROCM_CONVERSIONS
   registry.insert<ROCDL::ROCDLDialect>();
 #endif
diff --git a/mlir/lib/Target/LLVM/NVVM/Target.cpp b/mlir/lib/Target/LLVM/NVVM/Target.cpp
index acb903aa37caf..c32ccf90cbbbc 100644
--- a/mlir/lib/Target/LLVM/NVVM/Target.cpp
+++ b/mlir/lib/Target/LLVM/NVVM/Target.cpp
@@ -157,7 +157,6 @@ SerializeGPUModuleBase::loadBitcodeFiles(llvm::Module &module) {
   return std::move(bcFiles);
 }
 
-#if LLVM_HAS_NVPTX_TARGET
 namespace {
 class NVPTXSerializer : public SerializeGPUModuleBase {
 public:
@@ -532,6 +531,12 @@ NVPTXSerializer::moduleToObject(llvm::Module &llvmModule) {
   if (targetOptions.getCompilationTarget() == gpu::CompilationTarget::Offload)
     return SerializeGPUModuleBase::moduleToObject(llvmModule);
 
+#if ! LLVM_HAS_NVPTX_TARGET
+  getOperation()->emitError(
+      "The `NVPTX` target was not built. Please enable it when building LLVM.");
+  return std::nullopt;
+#endif // LLVM_HAS_NVPTX_TARGET
+
   // Emit PTX code.
   std::optional<llvm::TargetMachine *> targetMachine =
       getOrCreateTargetMachine();
@@ -569,7 +574,6 @@ NVPTXSerializer::moduleToObject(llvm::Module &llvmModule) {
   return compileToBinary(*serializedISA);
 #endif // MLIR_ENABLE_NVPTXCOMPILER
 }
-#endif // LLVM_HAS_NVPTX_TARGET
 
 std::optional<SmallVector<char, 0>>
 NVVMTargetAttrImpl::serializeToObject(Attribute attribute, Operation *module,
@@ -581,15 +585,9 @@ NVVMTargetAttrImpl::serializeToObject(Attribute attribute, Operation *module,
     module->emitError("Module must be a GPU module.");
     return std::nullopt;
   }
-#if LLVM_HAS_NVPTX_TARGET
   NVPTXSerializer serializer(*module, cast<NVVMTargetAttr>(attribute), options);
   serializer.init();
   return serializer.run();
-#else
-  module->emitError(
-      "The `NVPTX` target was not built. Please enable it when building LLVM.");
-  return std::nullopt;
-#endif // LLVM_HAS_NVPTX_TARGET
 }
 
 Attribute

Copy link

github-actions bot commented Jul 1, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Contributor

@fabianmcg fabianmcg left a comment

Choose a reason for hiding this comment

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

LGTM, see minor comment on ModuleToBinary

Most of the code here does not depend on the NVPTX target. In particular
the simple offload can just emit LLVM IR and we can use this without
the NVVM backend being built, which can be useful for a frontend that
just need to serialize the IR and leave it up to the runtime to JIT
further.
@joker-eph joker-eph merged commit d7da0ae into llvm:main Jul 2, 2024
7 checks passed
@joker-eph joker-eph deleted the nvptx-guards2 branch July 2, 2024 09:51
lravenclaw pushed a commit to lravenclaw/llvm-project that referenced this pull request Jul 3, 2024
…m#97349)

Most of the code here does not depend on the NVPTX target. In particular
the simple offload can just emit LLVM IR and we can use this without the
NVVM backend being built, which can be useful for a frontend that just
need to serialize the IR and leave it up to the runtime to JIT further.
kbluck pushed a commit to kbluck/llvm-project that referenced this pull request Jul 6, 2024
…m#97349)

Most of the code here does not depend on the NVPTX target. In particular
the simple offload can just emit LLVM IR and we can use this without the
NVVM backend being built, which can be useful for a frontend that just
need to serialize the IR and leave it up to the runtime to JIT further.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants