Skip to content

[MLIR][NVVM] Make the call to findTool optional for fatbinary #93968

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
May 31, 2024

Conversation

apaszke
Copy link
Member

@apaszke apaszke commented May 31, 2024

The fatbinary tool is only needed when the fatbinary format is requested.

The current code is especially confusing in that it emits an error complaining that fatbinary has not been found even though it's not even used (and the serialization succeeds nevertheless...).

The fatbinary tool is only needed when the fatbinary format is requested.

The current code is especially confusing in that it emits an error
complaining that fatbinary has not been found even though it's not
even used (and the serialization succeeds nevertheless...).
@llvmbot
Copy link
Member

llvmbot commented May 31, 2024

@llvm/pr-subscribers-mlir-llvm

@llvm/pr-subscribers-mlir

Author: Adam Paszke (apaszke)

Changes

The fatbinary tool is only needed when the fatbinary format is requested.

The current code is especially confusing in that it emits an error complaining that fatbinary has not been found even though it's not even used (and the serialization succeeds nevertheless...).


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

1 Files Affected:

  • (modified) mlir/lib/Target/LLVM/NVVM/Target.cpp (+6-3)
diff --git a/mlir/lib/Target/LLVM/NVVM/Target.cpp b/mlir/lib/Target/LLVM/NVVM/Target.cpp
index e75547ff9b850..acb903aa37caf 100644
--- a/mlir/lib/Target/LLVM/NVVM/Target.cpp
+++ b/mlir/lib/Target/LLVM/NVVM/Target.cpp
@@ -267,9 +267,12 @@ NVPTXSerializer::compileToBinary(const std::string &ptxCode) {
   std::optional<std::string> ptxasCompiler = findTool("ptxas");
   if (!ptxasCompiler)
     return std::nullopt;
-  std::optional<std::string> fatbinaryTool = findTool("fatbinary");
-  if (createFatbin && !fatbinaryTool)
-    return std::nullopt;
+  std::optional<std::string> fatbinaryTool;
+  if (createFatbin) {
+    fatbinaryTool = findTool("fatbinary");
+    if (!fatbinaryTool)
+      return std::nullopt;
+  }
   Location loc = getOperation().getLoc();
 
   // Base name for all temp files: mlir-<module name>-<target triple>-<chip>.

@grypp grypp requested a review from fabianmcg May 31, 2024 15:50
@grypp
Copy link
Member

grypp commented May 31, 2024

Looks good to me. Let's also check with @fabianmcg

Copy link
Collaborator

@joker-eph joker-eph left a comment

Choose a reason for hiding this comment

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

LG, thanks!

@grypp grypp merged commit 3320249 into llvm:main May 31, 2024
10 checks passed
@grypp
Copy link
Member

grypp commented May 31, 2024

Took another look, the change is actually trivial. So I merged it.
Thanks for the fix

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.

4 participants