[mlir][ROCDL] Fix file leak in SeralizeToHsaco and its newer form

[krzysz00]

SerializetToHsaco, as currently implemented, leaks the file descriptor of the .hsaco temporary file, which causes issues in long-running parallel compilation setups.

See also ROCm/rocMLIR#1257

[llvmbot]

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

@llvm/pr-subscribers-mlir-gpu

Changes

SerializetToHsaco, as currently implemented, leaks the file descriptor of the .hsaco temporary file, which causes issues in long-running parallel compilation setups.

See also ROCm/rocMLIR#1257

---

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

2 Files Affected:

• (modified) mlir/lib/Dialect/GPU/Transforms/SerializeToHsaco.cpp (+6-3)
• (modified) mlir/lib/Target/LLVM/ROCDL/Target.cpp (+4-2)

diff --git a/mlir/lib/Dialect/GPU/Transforms/SerializeToHsaco.cpp b/mlir/lib/Dialect/GPU/Transforms/SerializeToHsaco.cpp
index 9db0a942f7c38fd..dcf3bf016eee438 100644
--- a/mlir/lib/Dialect/GPU/Transforms/SerializeToHsaco.cpp
+++ b/mlir/lib/Dialect/GPU/Transforms/SerializeToHsaco.cpp
@@ -12,7 +12,6 @@
 //===----------------------------------------------------------------------===//
 
 #include "mlir/Dialect/GPU/Transforms/Passes.h"
-#include "mlir/Dialect/LLVMIR/ROCDLDialect.h"
 #include "mlir/IR/Location.h"
 #include "mlir/IR/MLIRContext.h"
 
@@ -43,6 +42,7 @@
 #include "llvm/MC/TargetRegistry.h"
 
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/FileSystem.h"
 #include "llvm/Support/FileUtilities.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Program.h"
@@ -410,6 +410,8 @@ SerializeToHsacoPass::createHsaco(const SmallVectorImpl<char> &isaBinary) {
     return {};
   }
   llvm::FileRemover cleanupHsaco(tempHsacoFilename);
+  // Prevent file descriptor leak.
+  llvm::sys::fs::closeFile(tempHsacoFD);
 
   std::string theRocmPath = getRocmPath();
   llvm::SmallString<32> lldPath(theRocmPath);
@@ -423,13 +425,14 @@ SerializeToHsacoPass::createHsaco(const SmallVectorImpl<char> &isaBinary) {
   }
 
   // Load the HSA code object.
-  auto hsacoFile = openInputFile(tempHsacoFilename);
+  auto hsacoFile =
+      llvm::MemoryBuffer::getFile(tempHsacoFilename, /*IsText=*/false);
   if (!hsacoFile) {
     emitError(loc, "read HSA code object from temp file error");
     return {};
   }
 
-  StringRef buffer = hsacoFile->getBuffer();
+  StringRef buffer = (*hsacoFile)->getBuffer();
   return std::make_unique<std::vector<char>>(buffer.begin(), buffer.end());
 }
 
diff --git a/mlir/lib/Target/LLVM/ROCDL/Target.cpp b/mlir/lib/Target/LLVM/ROCDL/Target.cpp
index 611d08fe3e79e56..c6f2ee1e4722df5 100644
--- a/mlir/lib/Target/LLVM/ROCDL/Target.cpp
+++ b/mlir/lib/Target/LLVM/ROCDL/Target.cpp
@@ -386,6 +386,7 @@ AMDGPUSerializer::compileToBinary(const std::string &serializedISA) {
     return std::nullopt;
   }
   llvm::FileRemover cleanupHsaco(tempHsacoFilename);
+  llvm::sys::fs::closeFile(tempHsacoFD);
 
   llvm::SmallString<128> lldPath(toolkitPath);
   llvm::sys::path::append(lldPath, "llvm", "bin", "ld.lld");
@@ -398,14 +399,15 @@ AMDGPUSerializer::compileToBinary(const std::string &serializedISA) {
   }
 
   // Load the HSA code object.
-  auto hsacoFile = openInputFile(tempHsacoFilename);
+  auto hsacoFile =
+      llvm::MemoryBuffer::getFile(tempHsacoFilename, /*IsText=*/false);
   if (!hsacoFile) {
     getOperation().emitError()
         << "Failed to read the HSA code object from the temp file.";
     return std::nullopt;
   }
 
-  StringRef buffer = hsacoFile->getBuffer();
+  StringRef buffer = (*hsacoFile)->getBuffer();
 
   return SmallVector<char, 0>(buffer.begin(), buffer.end());
 }

[fabianmcg]

LGTM!

[fabianmcg]

The Windows build is failing because llvm::sys::fs::closeFile requires a native file handle.

[fabianmcg]

Suggested change

	llvm::sys::fs::closeFile(tempHsacoFD);
	llvm::sys::fs::closeFile(llvm::sys::fs::convertFDToNativeFile(tempHsacoFD));

[joker-eph]

There is a createTemporaryFile that does not take a fd, why not use this one instead?

[krzysz00]

I didn't know that

[fabianmcg]

Suggested change

	llvm::sys::fs::closeFile(tempHsacoFD);
	llvm::sys::fs::closeFile(llvm::sys::fs::convertFDToNativeFile(tempHsacoFD));