Make SmallVectorImpl destructor protected #71439

andykaylor · 2023-11-06T20:02:59Z

Because the SmallVectorImpl destructor is not virtual, the destructor of derived classes will not be called if pointers to the SmallVectorImpl class are deleted directly. Making the SmallVectorImpl destructor protected will prevent this.

llvmbot · 2023-11-06T20:03:29Z

@llvm/pr-subscribers-mlir-gpu

@llvm/pr-subscribers-llvm-adt

Author: Andy Kaylor (andykaylor)

Changes

Because the SmallVectorImpl destructor is not virtual, the destructor of derived classes will not be called if pointers to the SmallVectorImpl class are deleted directly. Making the SmallVectorImpl destructor protected will prevent this.

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

2 Files Affected:

(modified) llvm/include/llvm/ADT/SmallVector.h (+3-3)
(modified) mlir/lib/Dialect/GPU/Transforms/SerializeToHsaco.cpp (+3-3)

diff --git a/llvm/include/llvm/ADT/SmallVector.h b/llvm/include/llvm/ADT/SmallVector.h
index 53a107b1574c6a3..2e6d2dc6ce90a2c 100644
--- a/llvm/include/llvm/ADT/SmallVector.h
+++ b/llvm/include/llvm/ADT/SmallVector.h
@@ -601,9 +601,6 @@ class SmallVectorImpl : public SmallVectorTemplateBase<T> {
     RHS.resetToSmall();
   }
 
-public:
-  SmallVectorImpl(const SmallVectorImpl &) = delete;
-
   ~SmallVectorImpl() {
     // Subclass has already destructed this vector's elements.
     // If this wasn't grown from the inline copy, deallocate the old space.
@@ -611,6 +608,9 @@ class SmallVectorImpl : public SmallVectorTemplateBase<T> {
       free(this->begin());
   }
 
+public:
+  SmallVectorImpl(const SmallVectorImpl &) = delete;
+
   void clear() {
     this->destroy_range(this->begin(), this->end());
     this->Size = 0;
diff --git a/mlir/lib/Dialect/GPU/Transforms/SerializeToHsaco.cpp b/mlir/lib/Dialect/GPU/Transforms/SerializeToHsaco.cpp
index 5c288e977ad0f23..97b3b66eda00133 100644
--- a/mlir/lib/Dialect/GPU/Transforms/SerializeToHsaco.cpp
+++ b/mlir/lib/Dialect/GPU/Transforms/SerializeToHsaco.cpp
@@ -95,7 +95,7 @@ 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<SmallVector<char, 16>> assembleIsa(const std::string &isa);
   std::unique_ptr<std::vector<char>>
   createHsaco(const SmallVectorImpl<char> &isaBinary);
 
@@ -318,7 +318,7 @@ SerializeToHsacoPass::translateToLLVMIR(llvm::LLVMContext &llvmContext) {
   return ret;
 }
 
-std::unique_ptr<SmallVectorImpl<char>>
+std::unique_ptr<SmallVector<char, 16>>
 SerializeToHsacoPass::assembleIsa(const std::string &isa) {
   auto loc = getOperation().getLoc();
 
@@ -381,7 +381,7 @@ SerializeToHsacoPass::assembleIsa(const std::string &isa) {
   parser->setTargetParser(*tap);
   parser->Run(false);
 
-  return std::make_unique<SmallVector<char, 0>>(std::move(result));
+  return std::make_unique<SmallVector<char, 16>>(std::move(result));
 }
 
 std::unique_ptr<std::vector<char>>

llvmbot · 2023-11-06T20:03:29Z

@llvm/pr-subscribers-mlir

Author: Andy Kaylor (andykaylor)

Changes

Because the SmallVectorImpl destructor is not virtual, the destructor of derived classes will not be called if pointers to the SmallVectorImpl class are deleted directly. Making the SmallVectorImpl destructor protected will prevent this.

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

2 Files Affected:

(modified) llvm/include/llvm/ADT/SmallVector.h (+3-3)
(modified) mlir/lib/Dialect/GPU/Transforms/SerializeToHsaco.cpp (+3-3)

diff --git a/llvm/include/llvm/ADT/SmallVector.h b/llvm/include/llvm/ADT/SmallVector.h
index 53a107b1574c6a3..2e6d2dc6ce90a2c 100644
--- a/llvm/include/llvm/ADT/SmallVector.h
+++ b/llvm/include/llvm/ADT/SmallVector.h
@@ -601,9 +601,6 @@ class SmallVectorImpl : public SmallVectorTemplateBase<T> {
     RHS.resetToSmall();
   }
 
-public:
-  SmallVectorImpl(const SmallVectorImpl &) = delete;
-
   ~SmallVectorImpl() {
     // Subclass has already destructed this vector's elements.
     // If this wasn't grown from the inline copy, deallocate the old space.
@@ -611,6 +608,9 @@ class SmallVectorImpl : public SmallVectorTemplateBase<T> {
       free(this->begin());
   }
 
+public:
+  SmallVectorImpl(const SmallVectorImpl &) = delete;
+
   void clear() {
     this->destroy_range(this->begin(), this->end());
     this->Size = 0;
diff --git a/mlir/lib/Dialect/GPU/Transforms/SerializeToHsaco.cpp b/mlir/lib/Dialect/GPU/Transforms/SerializeToHsaco.cpp
index 5c288e977ad0f23..97b3b66eda00133 100644
--- a/mlir/lib/Dialect/GPU/Transforms/SerializeToHsaco.cpp
+++ b/mlir/lib/Dialect/GPU/Transforms/SerializeToHsaco.cpp
@@ -95,7 +95,7 @@ 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<SmallVector<char, 16>> assembleIsa(const std::string &isa);
   std::unique_ptr<std::vector<char>>
   createHsaco(const SmallVectorImpl<char> &isaBinary);
 
@@ -318,7 +318,7 @@ SerializeToHsacoPass::translateToLLVMIR(llvm::LLVMContext &llvmContext) {
   return ret;
 }
 
-std::unique_ptr<SmallVectorImpl<char>>
+std::unique_ptr<SmallVector<char, 16>>
 SerializeToHsacoPass::assembleIsa(const std::string &isa) {
   auto loc = getOperation().getLoc();
 
@@ -381,7 +381,7 @@ SerializeToHsacoPass::assembleIsa(const std::string &isa) {
   parser->setTargetParser(*tap);
   parser->Run(false);
 
-  return std::make_unique<SmallVector<char, 0>>(std::move(result));
+  return std::make_unique<SmallVector<char, 16>>(std::move(result));
 }
 
 std::unique_ptr<std::vector<char>>

kuhar

Overall I think this makes sense, but the SerializeToHsaco changes look suboptimal

kuhar · 2023-11-06T20:15:50Z

mlir/lib/Dialect/GPU/Transforms/SerializeToHsaco.cpp

@@ -95,7 +95,7 @@ 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<SmallVector<char, 16>> assembleIsa(const std::string &isa);


IMO this should either return SmallVector<char> (without unique_ptr and an exact size) or take SmallVectorImpl<char> & as the second function argument

I'm not familiar enough with this code to know what would be best, and honestly I didn't spend any time trying to figure that out. My goal was just to get it to compile with my SmallVectorImpl change. I hope that the maintainers of this code will make a change that will make my change unnecessary.

@krzysz00 can you change this ahead of the current PR?

dwblaikie · 2023-11-07T17:47:41Z

We probably (pretty sure) don't want to add a virtual dtor to SmallVector - that'd add a vtable pointer, increasing the size in ways that are probably unacceptable given the pervasive use of the data structure.

We should make the Impl dtor protected so it can't be polymorphically destroyed.

andykaylor · 2023-11-07T18:29:00Z

We probably (pretty sure) don't want to add a virtual dtor to SmallVector - that'd add a vtable pointer, increasing the size in ways that are probably unacceptable given the pervasive use of the data structure.

We should make the Impl dtor protected so it can't be polymorphically destroyed.

That's what I thought as well. It's not particularly clear since GitHub is highlighting the deleted constructor as the thing that changed, but that is what this PR does -- the "public" line is dropped below the destructor, making it protected.

kuhar

I think we have reached consensus that this is the preferred direction. SerializeToHsaco is a tiny detail in the grand scheme of things, and because this PR doesn't make it any worse, I don't think we should be blocked by it.

krzysz00 · 2023-11-07T19:23:49Z

I don't know if I'll be able to get to the SerializeToHsaco fix today, but passing in SmallVectorImpl<char>& would be my preferred solution ...

Or really that should be MemoryBuffer & or some other such structure if feasible.

andykaylor · 2023-11-07T19:30:38Z

I don't think this change is time-critical. I can wait a few days for the SerializeToHsaco fix, but if it's going to be more than a few days, I'd prefer to land this change with an interim fix in SerializeToHsaco. @krzysz00 what's your preference?

krzysz00 · 2023-11-08T16:59:50Z

I put up a PR to fix SerializeToHsaco and unblock this

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>& .

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>& .

andykaylor · 2023-11-08T21:47:52Z

I'm not sure what happened here. I reset the branch in my fork so I could update this with the #71702 change in place. I'm testing locally and plan to update this PR (or create a new one) when I'm done.

joker-eph · 2023-11-09T00:15:13Z

You can always force-push here the same branch you pushed for the new PR: that’ll set the same state.

I think we have reached consensus that this is the preferred direction. SerializeToHsaco is a tiny detail in the grand scheme of things, and because this PR doesn't make it any worse, I don't think we should be blocked by it.

This is solved, but to keep in mind for next time: this is the kind of change to land separately.
First because we can, but second because this PR could break something else and be reverted / relabeled a couple of times. No need to couple the changes and add churn in the other component (here MLIR). Also it can increase the chance of conflicts if someone wanted to revert (because of evolution in the other parts of the code): so just good practices :)

andykaylor · 2023-11-09T00:44:48Z

This is solved, but to keep in mind for next time: this is the kind of change to land separately. First because we can, but second because this PR could break something else and be reverted / relabeled a couple of times. No need to couple the changes and add churn in the other component (here MLIR). Also it can increase the chance of conflicts if someone wanted to revert (because of evolution in the other parts of the code): so just good practices :)

Sure, that makes a lot of sense, and I didn't really expect this to land with the MLIR changes as part of my commit. I was just including them in the first revision because otherwise my change would have made things unbuildable.

andykaylor requested a review from joker-eph November 6, 2023 20:02

llvmbot added mlir:gpu mlir llvm:adt labels Nov 6, 2023

kuhar reviewed Nov 6, 2023

View reviewed changes

kuhar approved these changes Nov 7, 2023

View reviewed changes

krzysz00 mentioned this pull request Nov 8, 2023

Fix SmallVector usage in SerailzeToHsaco #71702

Merged

andykaylor closed this Nov 8, 2023

andykaylor force-pushed the protect-svi-destructor branch from 7d56022 to ef6d187 Compare November 8, 2023 21:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make SmallVectorImpl destructor protected #71439

Make SmallVectorImpl destructor protected #71439

andykaylor commented Nov 6, 2023

llvmbot commented Nov 6, 2023 •

edited

llvmbot commented Nov 6, 2023

kuhar left a comment

kuhar Nov 6, 2023 •

edited

andykaylor Nov 6, 2023

joker-eph Nov 7, 2023

dwblaikie commented Nov 7, 2023

andykaylor commented Nov 7, 2023

kuhar left a comment

krzysz00 commented Nov 7, 2023

andykaylor commented Nov 7, 2023

krzysz00 commented Nov 8, 2023

andykaylor commented Nov 8, 2023

joker-eph commented Nov 9, 2023

andykaylor commented Nov 9, 2023

Make SmallVectorImpl destructor protected #71439

Make SmallVectorImpl destructor protected #71439

Conversation

andykaylor commented Nov 6, 2023

llvmbot commented Nov 6, 2023 • edited

llvmbot commented Nov 6, 2023

kuhar left a comment

Choose a reason for hiding this comment

kuhar Nov 6, 2023 • edited

Choose a reason for hiding this comment

andykaylor Nov 6, 2023

Choose a reason for hiding this comment

joker-eph Nov 7, 2023

Choose a reason for hiding this comment

dwblaikie commented Nov 7, 2023

andykaylor commented Nov 7, 2023

kuhar left a comment

Choose a reason for hiding this comment

krzysz00 commented Nov 7, 2023

andykaylor commented Nov 7, 2023

krzysz00 commented Nov 8, 2023

andykaylor commented Nov 8, 2023

joker-eph commented Nov 9, 2023

andykaylor commented Nov 9, 2023

llvmbot commented Nov 6, 2023 •

edited

kuhar Nov 6, 2023 •

edited