Skip to content

Fix SmallVector usage in SerailzeToHsaco #71702

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

Conversation

krzysz00
Copy link
Contributor

@krzysz00 krzysz00 commented Nov 8, 2023

Enable merging #71439 by removing a definitely-wrong usage of std::unique_ptr<SmallVectorImpl> as a return value with passing in a SmallVectorImpl&

Also change the following function to take ArrayRef instead of const SmalVectorImpl& .

Enable merging llvm#71439 by removing a definitely-wrong usage of
std::unique_ptr<SmallVectorImpl<char>> as a return value with passing
in a SmallVectorImpl<char>&

Also change the following function to take ArrayRef<char> instead of
const SmalVectorImpl<char>& .
@llvmbot
Copy link
Member

llvmbot commented Nov 8, 2023

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-gpu

Author: Krzysztof Drewniak (krzysz00)

Changes

Enable merging #71439 by removing a definitely-wrong usage of std::unique_ptr<SmallVectorImpl<char>> as a return value with passing in a SmallVectorImpl<char>&

Also change the following function to take ArrayRef<char> instead of const SmalVectorImpl<char>& .


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

1 Files Affected:

  • (modified) mlir/lib/Dialect/GPU/Transforms/SerializeToHsaco.cpp (+14-19)
diff --git a/mlir/lib/Dialect/GPU/Transforms/SerializeToHsaco.cpp b/mlir/lib/Dialect/GPU/Transforms/SerializeToHsaco.cpp
index 5c288e977ad0f23..f0c294c22695b53 100644
--- a/mlir/lib/Dialect/GPU/Transforms/SerializeToHsaco.cpp
+++ b/mlir/lib/Dialect/GPU/Transforms/SerializeToHsaco.cpp
@@ -95,9 +95,9 @@ class SerializeToHsacoPass
   std::unique_ptr<std::vector<char>>
   serializeISA(const std::string &isa) override;
 
-  std::unique_ptr<SmallVectorImpl<char>> assembleIsa(const std::string &isa);
-  std::unique_ptr<std::vector<char>>
-  createHsaco(const SmallVectorImpl<char> &isaBinary);
+  LogicalResult assembleIsa(const std::string &isa,
+                            SmallVectorImpl<char> &result);
+  std::unique_ptr<std::vector<char>> createHsaco(ArrayRef<char> isaBinary);
 
   std::string getRocmPath();
 };
@@ -318,21 +318,18 @@ SerializeToHsacoPass::translateToLLVMIR(llvm::LLVMContext &llvmContext) {
   return ret;
 }
 
-std::unique_ptr<SmallVectorImpl<char>>
-SerializeToHsacoPass::assembleIsa(const std::string &isa) {
+LogicalResult SerializeToHsacoPass::assembleIsa(const std::string &isa,
+                                                SmallVectorImpl<char> &result) {
   auto loc = getOperation().getLoc();
 
-  SmallVector<char, 0> result;
   llvm::raw_svector_ostream os(result);
 
   llvm::Triple triple(llvm::Triple::normalize(this->triple));
   std::string error;
   const llvm::Target *target =
       llvm::TargetRegistry::lookupTarget(triple.normalize(), error);
-  if (!target) {
-    emitError(loc, Twine("failed to lookup target: ") + error);
-    return {};
-  }
+  if (!target)
+    return emitError(loc, Twine("failed to lookup target: ") + error);
 
   llvm::SourceMgr srcMgr;
   srcMgr.AddNewSourceBuffer(llvm::MemoryBuffer::getMemBuffer(isa), SMLoc());
@@ -373,19 +370,17 @@ SerializeToHsacoPass::assembleIsa(const std::string &isa) {
   std::unique_ptr<llvm::MCTargetAsmParser> tap(
       target->createMCAsmParser(*sti, *parser, *mcii, mcOptions));
 
-  if (!tap) {
-    emitError(loc, "assembler initialization error");
-    return {};
-  }
+  if (!tap)
+    return emitError(loc, "assembler initialization error");
 
   parser->setTargetParser(*tap);
   parser->Run(false);
 
-  return std::make_unique<SmallVector<char, 0>>(std::move(result));
+  return success();
 }
 
 std::unique_ptr<std::vector<char>>
-SerializeToHsacoPass::createHsaco(const SmallVectorImpl<char> &isaBinary) {
+SerializeToHsacoPass::createHsaco(ArrayRef<char> isaBinary) {
   auto loc = getOperation().getLoc();
 
   // Save the ISA binary to a temp file.
@@ -435,10 +430,10 @@ SerializeToHsacoPass::createHsaco(const SmallVectorImpl<char> &isaBinary) {
 
 std::unique_ptr<std::vector<char>>
 SerializeToHsacoPass::serializeISA(const std::string &isa) {
-  auto isaBinary = assembleIsa(isa);
-  if (!isaBinary)
+  SmallVector<char, 0> isaBinary;
+  if (failed(assembleIsa(isa, isaBinary)))
     return {};
-  return createHsaco(*isaBinary);
+  return createHsaco(isaBinary);
 }
 
 // Register pass to serialize GPU kernel functions to a HSACO binary annotation.

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 but fix the title please :)

@krzysz00 krzysz00 changed the title Fix SmallVector usage in SerailezToHsaco Fix SmallVector usage in SerailzeToHsaco Nov 8, 2023
@krzysz00 krzysz00 merged commit 05fa923 into llvm:main Nov 8, 2023
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