Skip to content
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

[mlir][nvvm] Expose MLIR_NVPTXCOMPILER_ENABLED in mlir-config.h. #84007

Merged
merged 1 commit into from
Mar 6, 2024

Conversation

ingomueller-net
Copy link
Contributor

This is another follow-up of #83004, which made the same change for MLIR_CUDA_CONVERSIONS_ENABLED. As the previous PR, this PR commit exposes mentioned CMake variable through mlir-config.h and uses the macro that is introduced with the same name. This replaces the macro MLIR_NVPTXCOMPILER_ENABLED, which the CMake files previously defined manually.

@llvmbot
Copy link
Collaborator

llvmbot commented Mar 5, 2024

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-llvm

Author: Ingo Müller (ingomueller-net)

Changes

This is another follow-up of #83004, which made the same change for MLIR_CUDA_CONVERSIONS_ENABLED. As the previous PR, this PR commit exposes mentioned CMake variable through mlir-config.h and uses the macro that is introduced with the same name. This replaces the macro MLIR_NVPTXCOMPILER_ENABLED, which the CMake files previously defined manually.


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

4 Files Affected:

  • (modified) mlir/include/mlir/Config/mlir-config.h.cmake (+3)
  • (modified) mlir/lib/Target/LLVM/CMakeLists.txt (-1)
  • (modified) mlir/lib/Target/LLVM/NVVM/Target.cpp (+6-5)
  • (modified) utils/bazel/llvm-project-overlay/mlir/BUILD.bazel (+9-2)
diff --git a/mlir/include/mlir/Config/mlir-config.h.cmake b/mlir/include/mlir/Config/mlir-config.h.cmake
index 4a7d75e2266823..03fda997313ba9 100644
--- a/mlir/include/mlir/Config/mlir-config.h.cmake
+++ b/mlir/include/mlir/Config/mlir-config.h.cmake
@@ -33,4 +33,7 @@
    and targets. */
 #cmakedefine01 MLIR_ENABLE_CUDA_CONVERSIONS
 
+/* If set, enables features that depend on the NVIDIA's PTX compiler. */
+#cmakedefine01 MLIR_ENABLE_NVPTXCOMPILER
+
 #endif
diff --git a/mlir/lib/Target/LLVM/CMakeLists.txt b/mlir/lib/Target/LLVM/CMakeLists.txt
index cc2c3a00a02eaf..e0657c895e8a38 100644
--- a/mlir/lib/Target/LLVM/CMakeLists.txt
+++ b/mlir/lib/Target/LLVM/CMakeLists.txt
@@ -93,7 +93,6 @@ if(MLIR_ENABLE_CUDA_CONVERSIONS)
   # Define the `CUDAToolkit` path.
   target_compile_definitions(obj.MLIRNVVMTarget
     PRIVATE
-    MLIR_NVPTXCOMPILER_ENABLED=${MLIR_ENABLE_NVPTXCOMPILER}
     __DEFAULT_CUDATOOLKIT_PATH__="${MLIR_CUDAToolkit_ROOT}"
   )
 endif()
diff --git a/mlir/lib/Target/LLVM/NVVM/Target.cpp b/mlir/lib/Target/LLVM/NVVM/Target.cpp
index d5b6645631edd6..e438ce84af1b5f 100644
--- a/mlir/lib/Target/LLVM/NVVM/Target.cpp
+++ b/mlir/lib/Target/LLVM/NVVM/Target.cpp
@@ -22,6 +22,7 @@
 #include "mlir/Target/LLVMIR/Dialect/NVVM/NVVMToLLVMIRTranslation.h"
 #include "mlir/Target/LLVMIR/Export.h"
 
+#include "llvm/Config/llvm-config.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/FileUtilities.h"
 #include "llvm/Support/FormatVariadic.h"
@@ -431,7 +432,7 @@ NVPTXSerializer::compileToBinary(const std::string &ptxCode) {
   return SmallVector<char, 0>(fatbin.begin(), fatbin.end());
 }
 
-#if MLIR_NVPTXCOMPILER_ENABLED == 1
+#if MLIR_ENABLE_NVPTXCOMPILER
 #include "nvPTXCompiler.h"
 
 #define RETURN_ON_NVPTXCOMPILER_ERROR(expr)                                    \
@@ -510,7 +511,7 @@ NVPTXSerializer::compileToBinaryNVPTX(const std::string &ptxCode) {
   RETURN_ON_NVPTXCOMPILER_ERROR(nvPTXCompilerDestroy(&compiler));
   return binary;
 }
-#endif // MLIR_NVPTXCOMPILER_ENABLED == 1
+#endif // MLIR_ENABLE_NVPTXCOMPILER
 
 std::optional<SmallVector<char, 0>>
 NVPTXSerializer::moduleToObject(llvm::Module &llvmModule) {
@@ -556,12 +557,12 @@ NVPTXSerializer::moduleToObject(llvm::Module &llvmModule) {
     return SmallVector<char, 0>(bin.begin(), bin.end());
   }
 
-    // Compile to binary.
-#if MLIR_NVPTXCOMPILER_ENABLED == 1
+  // Compile to binary.
+#if MLIR_ENABLE_NVPTXCOMPILER
   return compileToBinaryNVPTX(*serializedISA);
 #else
   return compileToBinary(*serializedISA);
-#endif // MLIR_NVPTXCOMPILER_ENABLED == 1
+#endif // MLIR_ENABLE_NVPTXCOMPILER
 }
 #endif // MLIR_ENABLE_CUDA_CONVERSIONS
 
diff --git a/utils/bazel/llvm-project-overlay/mlir/BUILD.bazel b/utils/bazel/llvm-project-overlay/mlir/BUILD.bazel
index 8a8dd6e10c48aa..6875963905c49c 100644
--- a/utils/bazel/llvm-project-overlay/mlir/BUILD.bazel
+++ b/utils/bazel/llvm-project-overlay/mlir/BUILD.bazel
@@ -36,8 +36,14 @@ expand_template(
         "#cmakedefine MLIR_GREEDY_REWRITE_RANDOMIZER_SEED ${MLIR_GREEDY_REWRITE_RANDOMIZER_SEED}": "/* #undef MLIR_GREEDY_REWRITE_RANDOMIZER_SEED */",
         "#cmakedefine01 MLIR_ENABLE_PDL_IN_PATTERNMATCH": "#define MLIR_ENABLE_PDL_IN_PATTERNMATCH 1",
     } | if_cuda_available(
-        {"#cmakedefine01 MLIR_ENABLE_CUDA_CONVERSIONS": "#define MLIR_ENABLE_CUDA_CONVERSIONS 1"},
-        {"#cmakedefine01 MLIR_ENABLE_CUDA_CONVERSIONS": "#define MLIR_ENABLE_CUDA_CONVERSIONS 0"},
+        {
+            "#cmakedefine01 MLIR_ENABLE_CUDA_CONVERSIONS": "#define MLIR_ENABLE_CUDA_CONVERSIONS 1",
+            "#cmakedefine01 MLIR_ENABLE_NVPTXCOMPILER": "#define MLIR_ENABLE_NVPTXCOMPILER 1",
+        },
+        {
+            "#cmakedefine01 MLIR_ENABLE_CUDA_CONVERSIONS": "#define MLIR_ENABLE_CUDA_CONVERSIONS 0",
+            "#cmakedefine01 MLIR_ENABLE_NVPTXCOMPILER": "#define MLIR_ENABLE_NVPTXCOMPILER 0",
+        },
     ),
     template = "include/mlir/Config/mlir-config.h.cmake",
 )
@@ -6182,6 +6188,7 @@ cc_library(
         ":config",
         "//llvm:NVPTXCodeGen",
         "//llvm:Support",
+        "//llvm:config",
     ],
 )
 

This is another follow-up of llvm#83004, which made the same change for
`MLIR_CUDA_CONVERSIONS_ENABLED`. As the previous PR, this PR commit
exposes mentioned CMake variable through `mlir-config.h` and uses the
macro that is introduced with the same name. This replaces the macro
`MLIR_NVPTXCOMPILER_ENABLED`, which the CMake files previously defined
manually.
@ingomueller-net ingomueller-net merged commit 099045a into llvm:main Mar 6, 2024
4 checks passed
@ingomueller-net ingomueller-net deleted the nvptx-compiler-macro branch March 6, 2024 13:14
mmilanifard added a commit to mmilanifard/llvm-project that referenced this pull request Mar 6, 2024
frgossen pushed a commit that referenced this pull request Mar 6, 2024
ingomueller-net added a commit to ingomueller-net/llvm-project that referenced this pull request Mar 6, 2024
was available. However, it turns out that the NVPTX compiler is not part
of every CUDA distribution, so `if_cuda_available` may evaluate to true
without that compiler being present, which breaks the build. This PR
thus sets the macro to 0 always (which was the behavior before llvm#84007).
ingomueller-net added a commit that referenced this pull request Mar 6, 2024
…e. (#84238)

was available. However, it turns out that the NVPTX compiler is not part
of every CUDA distribution, so `if_cuda_available` may evaluate to true
without that compiler being present, which breaks the build. This PR
thus sets the macro to 0 always (which was the behavior before #84007).
@ingomueller-net
Copy link
Contributor Author

On first thought, that doesn't sound plausible. How are the two macros related? If you're using cmake, then it is most likely rather caused by #84011.

I am not sure about this specific case. Very generally, we either need to define the macro, either in a file or through the compiler/cmake/bazel, or turn the #if into #ifdef.

@PeimingLiu
Copy link
Member

On first thought, that doesn't sound plausible. How are the two macros related? If you're using cmake, then it is most likely rather caused by #84011.

I am not sure about this specific case. Very generally, we either need to define the macro, either in a file or through the compiler/cmake/bazel, or turn the #if into #ifdef.

Sorry, I commented on the wrong PR, it should be caused by #84011

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.

None yet

4 participants