IR: Clarify ownership of ConstantDataSequentials, NFC

[dexonsmith]

Summary

Change ConstantDataSequential::Next to a
unique_ptr<ConstantDataSequential> and update CDSConstants to a
StringMap<unique_ptr<ConstantDataSequential>>, making the ownership
more obvious.

[dblaikie]

Is this the right patch/description?

[dexonsmith]

In D90083#2351569, @dblaikie wrote:

Is this the right patch/description?

Right description / wrong patch :/. Fixed!

[aprantl]

This looks like a good simplification.

[dblaikie]

llvm/lib/IR/Constants.cpp

2787–2788

This becomes a bit fussy to read, what about:

std::unique_ptr<ConstantDataSequential> *Entry = &Slot.second;
for (; *Entry; Entry = &Node->Next)
  if ((*Entry)->getType() == Ty)
    return Entry->get();

Does that do the same thing? Perhaps I've missed something/broken it in some way.

2795

I'd generally favor *Entry = std::make_unique... (less cognitive load - there's no suspicious raw new I feel compelled to double-check ("Is that reset function on a unique_ptr, or some other thing that might have more nuanced semantics on raw pointers, etc, etc")) but up to you

[dexonsmith]

Sorry @dblaikie, just saw now (after I pushed) that you commented, I'll read now and decided whether to revert or do a follow-up.

[dexonsmith]

llvm/lib/IR/Constants.cpp

2787–2788

Yes, that's a nice cleanup. Thanks for working that through. I'll fix it up in a follow-up.

2795

Good point, I'll update to this.

[dexonsmith]

Thanks for the review.

llvm/lib/IR/Constants.cpp

2787–2788

I cleaned up this (and the other loop) in https://reviews.llvm.org/D90198.

2795

Ah, I remember why I didn't have this:

error: 'operator new' is a protected member of 'llvm::ConstantData'

Thoughts on whether it's worth working through this?

[dblaikie]

llvm/lib/IR/Constants.cpp

2787–2788

Great!

2795

oh, fair enough - nothing to do then!

(it's possible to make "private-like" constructors for use with make_unique - you can do this by having a public constructor with an unconstructible parameter (eg:

class t1 {
  private:
    struct private_thing { };
  public:
    t1(private_thing);
    std::unique_ptr<t1> make_t1() {
      return std::make_unique<t1>(private_thing());
    }
};

(making it fully robust may require even more work - given the above, not sure if you can use templates to extract and then instantiate the "private_thing" type - I'd have to test more, and then maybe give it a private ctor and have it befriend t1.

But all of this isn't usually worth it, imho)

[dexonsmith]

llvm/lib/IR/Constants.cpp

2795

Right then; I left behind a comment in 52821f6a71a568f966427b627839d40641653757 to clarify for readers.

llvm/include/llvm/IR/Constants.h

class ConstantDataSequential : public ConstantData {
   /// A pointer to the bytes underlying this constant (which is owned by the
   /// uniquing StringMap).
   const char *DataElements;
 
   /// This forms a link list of ConstantDataSequential nodes that have
   /// the same value but different type.  For example, 0,0,0,1 could be a 4
   /// element array of i8, or a 1-element array of i32.  They'll both end up in
   /// the same StringMap bucket, linked up.
-  ConstantDataSequential *Next;
+  std::unique_ptr<ConstantDataSequential> Next;
 
   void destroyConstantImpl();
 
 protected:
   explicit ConstantDataSequential(Type *ty, ValueTy VT, const char *Data)
-      : ConstantData(ty, VT), DataElements(Data), Next(nullptr) {}
-  ~ConstantDataSequential() { delete Next; }
+      : ConstantData(ty, VT), DataElements(Data) {}
 
   static Constant *getImpl(StringRef Bytes, Type *Ty);
 
 public:
   ConstantDataSequential(const ConstantDataSequential &) = delete;
 
   /// Return true if a ConstantDataSequential can be formed with a vector or
   /// array of the specified element type.

llvm/lib/IR/Constants.cpp

auto &Slot =
       *Ty->getContext()
            .pImpl->CDSConstants.insert(std::make_pair(Elements, nullptr))
            .first;
 
   // The bucket can point to a linked list of different CDS's that have the same
   // body but different types.  For example, 0,0,0,1 could be a 4 element array
   // of i8, or a 1-element array of i32.  They'll both end up in the same
   /// StringMap bucket, linked up by their Next pointers.  Walk the list.
-  ConstantDataSequential **Entry = &Slot.second;
-  for (ConstantDataSequential *Node = *Entry; Node;
-       Entry = &Node->Next, Node = *Entry)
+  std::unique_ptr<ConstantDataSequential> *Entry = &Slot.second;
+  for (ConstantDataSequential *Node = Entry->get(); Node;
+       Entry = &Node->Next, Node = Entry->get())
     if (Node->getType() == Ty)
       return Node;
 
   // Okay, we didn't get a hit.  Create a node of the right class, link it in,
   // and return it.
-  if (isa<ArrayType>(Ty))
-    return *Entry = new ConstantDataArray(Ty, Slot.first().data());
+  if (isa<ArrayType>(Ty)) {
+    Entry->reset(new ConstantDataArray(Ty, Slot.first().data()));
+    return Entry->get();
+  }
 
   assert(isa<VectorType>(Ty));
-  return *Entry = new ConstantDataVector(Ty, Slot.first().data());
+  Entry->reset(new ConstantDataVector(Ty, Slot.first().data()));
+  return Entry->get();
 }
 
 void ConstantDataSequential::destroyConstantImpl() {
   // Remove the constant from the StringMap.
-  StringMap<ConstantDataSequential*> &CDSConstants =
+  StringMap<std::unique_ptr<ConstantDataSequential>> &CDSConstants =
       getType()->getContext().pImpl->CDSConstants;
 
-  StringMap<ConstantDataSequential*>::iterator Slot =
-    CDSConstants.find(getRawDataValues());
+  auto Slot = CDSConstants.find(getRawDataValues());
 
   assert(Slot != CDSConstants.end() && "CDS not found in uniquing table");
 
-  ConstantDataSequential **Entry = &Slot->getValue();
+  std::unique_ptr<ConstantDataSequential> *Entry = &Slot->getValue();
 
   // Remove the entry from the hash table.
   if (!(*Entry)->Next) {
     // If there is only one value in the bucket (common case) it must be this
     // entry, and removing the entry should remove the bucket completely.
-    assert((*Entry) == this && "Hash mismatch in ConstantDataSequential");
+    assert(Entry->get() == this && "Hash mismatch in ConstantDataSequential");
     getContext().pImpl->CDSConstants.erase(Slot);
-  } else {
-    // Otherwise, there are multiple entries linked off the bucket, unlink the
-    // node we care about but keep the bucket around.
-    for (ConstantDataSequential *Node = *Entry; ;
-         Entry = &Node->Next, Node = *Entry) {
-      assert(Node && "Didn't find entry in its uniquing hash table!");
-      // If we found our entry, unlink it from the list and we're done.
-      if (Node == this) {
-        *Entry = Node->Next;
-        break;
-      }
+    return;
+  }
+
+  for (ConstantDataSequential *Node = Entry->get();;
+       Entry = &Node->Next, Node = Entry->get()) {
+      *Entry = std::move(Node->Next);
+      return;
     }
   }
-
-  // If we were part of a list, make sure that we don't delete the list that is
-  // still owned by the uniquing map.
-  Next = nullptr;
 }
 
 /// getFP() constructors - Return a constant of array type with a float
 /// element type taken from argument `ElementType', and count taken from
 /// argument `Elts'.  The amount of bits of the contained type must match the
 /// number of bits of the type contained in the passed in ArrayRef.
 /// (i.e. half or bfloat for 16bits, float for 32bits, double for 64bits) Note
 /// that this can return a ConstantAggregateZero object.

llvm/lib/IR/LLVMContextImpl.h

#include "llvm/IR/Metadata.def"
 
   using VectorConstantsTy = ConstantUniqueMap<ConstantVector>;
   VectorConstantsTy VectorConstants;
 
   DenseMap<PointerType *, std::unique_ptr<ConstantPointerNull>> CPNConstants;
 
   DenseMap<Type *, std::unique_ptr<UndefValue>> UVConstants;
 
-  StringMap<ConstantDataSequential*> CDSConstants;
+  StringMap<std::unique_ptr<ConstantDataSequential>> CDSConstants;
 
   DenseMap<std::pair<const Function *, const BasicBlock *>, BlockAddress *>
     BlockAddresses;
   ConstantUniqueMap<ConstantExpr> ExprConstants;
 
   ConstantUniqueMap<InlineAsm> InlineAsms;
 
   ConstantInt *TheTrueVal = nullptr;

llvm/lib/IR/LLVMContextImpl.cpp

#include "llvm/IR/Metadata.def"
   VectorConstants.freeConstants();
   InlineAsms.freeConstants();
 
   CAZConstants.clear();
   CPNConstants.clear();
   UVConstants.clear();
   IntConstants.clear();
   FPConstants.clear();
-
-  for (auto &CDSConstant : CDSConstants)
-    delete CDSConstant.second;
   CDSConstants.clear();
 
   // Destroy attribute node lists.
   for (FoldingSetIterator<AttributeSetNode> I = AttrsSetNodes.begin(),
          E = AttrsSetNodes.end(); I != E; ) {
     FoldingSetIterator<AttributeSetNode> Elem = I++;
     delete &*Elem;
   }