From b2b7cf39d596b1528cd64015575b3f5d1461c011 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Duncan=20P=2E=20N=2E=20Exon=C2=A0Smith?= Date: Fri, 23 Oct 2020 18:51:02 -0400 Subject: [PATCH] IR: Clarify ownership of ConstantDataSequentials, NFC Change `ConstantDataSequential::Next` to a `unique_ptr` and update `CDSConstants` to a `StringMap>`, making the ownership more obvious. Differential Revision: https://reviews.llvm.org/D90083 --- llvm/include/llvm/IR/Constants.h | 5 ++-- llvm/lib/IR/Constants.cpp | 53 ++++++++++++++++++++-------------------- llvm/lib/IR/LLVMContextImpl.cpp | 3 --- llvm/lib/IR/LLVMContextImpl.h | 2 +- 4 files changed, 29 insertions(+), 34 deletions(-) diff --git a/llvm/include/llvm/IR/Constants.h b/llvm/include/llvm/IR/Constants.h index 343702a..febcdb4 100644 --- a/llvm/include/llvm/IR/Constants.h +++ b/llvm/include/llvm/IR/Constants.h @@ -592,14 +592,13 @@ class ConstantDataSequential : public ConstantData { /// 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 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); diff --git a/llvm/lib/IR/Constants.cpp b/llvm/lib/IR/Constants.cpp index 7eca7dd..cfefd33 100644 --- a/llvm/lib/IR/Constants.cpp +++ b/llvm/lib/IR/Constants.cpp @@ -2783,56 +2783,55 @@ Constant *ConstantDataSequential::getImpl(StringRef Elements, Type *Ty) { // 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 *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(Ty)) - return *Entry = new ConstantDataArray(Ty, Slot.first().data()); + if (isa(Ty)) { + Entry->reset(new ConstantDataArray(Ty, Slot.first().data())); + return Entry->get(); + } assert(isa(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 &CDSConstants = - getType()->getContext().pImpl->CDSConstants; + StringMap> &CDSConstants = + getType()->getContext().pImpl->CDSConstants; - StringMap::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 *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; } - // 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; + // Otherwise, there are multiple entries linked off the bucket, unlink the + // node we care about but keep the bucket around. + for (ConstantDataSequential *Node = Entry->get();; + Entry = &Node->Next, Node = Entry->get()) { + 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 = std::move(Node->Next); + return; + } + } } /// getFP() constructors - Return a constant of array type with a float diff --git a/llvm/lib/IR/LLVMContextImpl.cpp b/llvm/lib/IR/LLVMContextImpl.cpp index 00f9971..c4f0a0a 100644 --- a/llvm/lib/IR/LLVMContextImpl.cpp +++ b/llvm/lib/IR/LLVMContextImpl.cpp @@ -99,9 +99,6 @@ LLVMContextImpl::~LLVMContextImpl() { UVConstants.clear(); IntConstants.clear(); FPConstants.clear(); - - for (auto &CDSConstant : CDSConstants) - delete CDSConstant.second; CDSConstants.clear(); // Destroy attribute node lists. diff --git a/llvm/lib/IR/LLVMContextImpl.h b/llvm/lib/IR/LLVMContextImpl.h index e676c5b..86514ff 100644 --- a/llvm/lib/IR/LLVMContextImpl.h +++ b/llvm/lib/IR/LLVMContextImpl.h @@ -1346,7 +1346,7 @@ public: DenseMap> UVConstants; - StringMap CDSConstants; + StringMap> CDSConstants; DenseMap, BlockAddress *> BlockAddresses; -- 2.7.4