From 8ec0aee3b4a0b038732f30226999145d666186f2 Mon Sep 17 00:00:00 2001 From: "Duncan P. N. Exon Smith" Date: Mon, 19 Jan 2015 20:36:39 +0000 Subject: [PATCH] IR: Remove MDNodeFwdDecl Remove `MDNodeFwdDecl` (as promised in r226481). Aside from API changes, there's no real functionality change here. `MDNode::getTemporary()` now forwards to `MDTuple::getTemporary()`, which returns a tuple with `isTemporary()` equal to true. The main point is that we can now add temporaries of other `MDNode` subclasses, needed for PR22235 (I introduced `MDNodeFwdDecl` in the first place because I didn't recognize this need, and thought they were only needed to handle forward references). A few things left out of (or highlighted by) this commit: - I've had to remove the (few) uses of `std::unique_ptr<>` to deal with temporaries, since the destructor is no longer public. `getTemporary()` should probably return the equivalent of `std::unique_ptr`. - `MDLocation::getTemporary()` doesn't exist yet (worse, it actually does exist, but does the wrong thing: `MDNode::getTemporary()` is inherited and returns an `MDTuple`). - `MDNode` now only has one subclass, `UniquableMDNode`, and the distinction between them is actually somewhat confusing. I'll fix those up next. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@226501 91177308-0d34-0410-b5e6-96231b3b80d8 --- bindings/go/llvm/IRBindings.cpp | 3 +- include/llvm/IR/DIBuilder.h | 2 +- include/llvm/IR/Metadata.def | 1 - include/llvm/IR/Metadata.h | 69 +++++++++---------------- lib/AsmParser/LLParser.cpp | 6 +-- lib/AsmParser/LLParser.h | 2 +- lib/Bitcode/Reader/BitcodeReader.cpp | 11 ++-- lib/IR/DebugInfo.cpp | 6 ++- lib/IR/Metadata.cpp | 24 ++++----- lib/IR/Verifier.cpp | 2 +- lib/Transforms/Utils/InlineFunction.cpp | 7 +-- lib/Transforms/Utils/ValueMapper.cpp | 8 +-- unittests/IR/MetadataTest.cpp | 31 ++++++----- 13 files changed, 77 insertions(+), 95 deletions(-) diff --git a/bindings/go/llvm/IRBindings.cpp b/bindings/go/llvm/IRBindings.cpp index fac4126acda..7c2fe60b2b9 100644 --- a/bindings/go/llvm/IRBindings.cpp +++ b/bindings/go/llvm/IRBindings.cpp @@ -86,7 +86,8 @@ void LLVMSetMetadata2(LLVMValueRef Inst, unsigned KindID, LLVMMetadataRef MD) { } void LLVMMetadataReplaceAllUsesWith(LLVMMetadataRef MD, LLVMMetadataRef New) { - auto *Node = unwrap(MD); + auto *Node = unwrap(MD); + assert(Node->isTemporary() && "Expected temporary node"); Node->replaceAllUsesWith(unwrap(New)); MDNode::deleteTemporary(Node); } diff --git a/include/llvm/IR/DIBuilder.h b/include/llvm/IR/DIBuilder.h index ae1ac650a9e..b0eec7e576b 100644 --- a/include/llvm/IR/DIBuilder.h +++ b/include/llvm/IR/DIBuilder.h @@ -85,7 +85,7 @@ namespace llvm { /// \brief Create a temporary. /// - /// Create an \a MDNodeFwdDecl and track it in \a UnresolvedNodes. + /// Create an \a temporary node and track it in \a UnresolvedNodes. void trackIfUnresolved(MDNode *N); public: diff --git a/include/llvm/IR/Metadata.def b/include/llvm/IR/Metadata.def index 2098bb57eb5..57cd773373e 100644 --- a/include/llvm/IR/Metadata.def +++ b/include/llvm/IR/Metadata.def @@ -47,7 +47,6 @@ HANDLE_METADATA_BRANCH(ValueAsMetadata) HANDLE_METADATA_LEAF(ConstantAsMetadata) HANDLE_METADATA_LEAF(LocalAsMetadata) HANDLE_METADATA_BRANCH(MDNode) -HANDLE_METADATA_LEAF(MDNodeFwdDecl) HANDLE_UNIQUABLE_BRANCH(UniquableMDNode) HANDLE_UNIQUABLE_LEAF(MDTuple) HANDLE_UNIQUABLE_LEAF(MDLocation) diff --git a/include/llvm/IR/Metadata.h b/include/llvm/IR/Metadata.h index bbbb328890f..6f02ee1e704 100644 --- a/include/llvm/IR/Metadata.h +++ b/include/llvm/IR/Metadata.h @@ -61,7 +61,6 @@ public: enum MetadataKind { MDTupleKind, MDLocationKind, - MDNodeFwdDeclKind, ConstantAsMetadataKind, LocalAsMetadataKind, MDStringKind @@ -698,14 +697,8 @@ public: ArrayRef MDs); static inline MDTuple *getDistinct(LLVMContext &Context, ArrayRef MDs); - - /// \brief Return a temporary MDNode - /// - /// For use in constructing cyclic MDNode structures. A temporary MDNode is - /// not uniqued, may be RAUW'd, and must be manually deleted with - /// deleteTemporary. - static MDNodeFwdDecl *getTemporary(LLVMContext &Context, - ArrayRef MDs); + static inline MDTuple *getTemporary(LLVMContext &Context, + ArrayRef MDs); /// \brief Deallocate a node created by getTemporary. /// @@ -772,8 +765,7 @@ public: /// \brief Methods for support type inquiry through isa, cast, and dyn_cast: static bool classof(const Metadata *MD) { return MD->getMetadataID() == MDTupleKind || - MD->getMetadataID() == MDLocationKind || - MD->getMetadataID() == MDNodeFwdDeclKind; + MD->getMetadataID() == MDLocationKind; } /// \brief Check whether MDNode is a vtable access. @@ -794,16 +786,15 @@ public: /// for implementing sub-types of \a MDNode that can be uniqued like /// constants. /// -/// There is limited support for RAUW at construction time. At -/// construction time, if any operands are an instance of \a -/// MDNodeFwdDecl (or another unresolved \a UniquableMDNode, which -/// indicates an \a MDNodeFwdDecl in its path), the node itself will be -/// unresolved. As soon as all operands become resolved, it will drop -/// RAUW support permanently. +/// There is limited support for RAUW at construction time. At construction +/// time, if any operand is a temporary node (or an unresolved uniqued node, +/// which indicates a transitive temporary operand), the node itself will be +/// unresolved. As soon as all operands become resolved, it will drop RAUW +/// support permanently. /// /// If an unresolved node is part of a cycle, \a resolveCycles() needs -/// to be called on some member of the cycle when each \a MDNodeFwdDecl -/// has been removed. +/// to be called on some member of the cycle once all temporary nodes have been +/// replaced. class UniquableMDNode : public MDNode { friend class ReplaceableMetadataImpl; friend class MDNode; @@ -834,7 +825,7 @@ public: /// Once all forward declarations have been resolved, force cycles to be /// resolved. /// - /// \pre No operands (or operands' operands, etc.) are \a MDNodeFwdDecl. + /// \pre No operands (or operands' operands, etc.) have \a isTemporary(). void resolveCycles(); private: @@ -888,6 +879,15 @@ public: return getImpl(Context, MDs, Distinct); } + /// \brief Return a temporary node. + /// + /// For use in constructing cyclic MDNode structures. A temporary MDNode is + /// not uniqued, may be RAUW'd, and must be manually deleted with + /// deleteTemporary. + static MDTuple *getTemporary(LLVMContext &Context, ArrayRef MDs) { + return getImpl(Context, MDs, Temporary); + } + static bool classof(const Metadata *MD) { return MD->getMetadataID() == MDTupleKind; } @@ -906,6 +906,9 @@ MDTuple *MDNode::getIfExists(LLVMContext &Context, ArrayRef MDs) { MDTuple *MDNode::getDistinct(LLVMContext &Context, ArrayRef MDs) { return MDTuple::getDistinct(Context, MDs); } +MDTuple *MDNode::getTemporary(LLVMContext &Context, ArrayRef MDs) { + return MDTuple::getTemporary(Context, MDs); +} /// \brief Debug location. /// @@ -961,32 +964,6 @@ private: void eraseFromStoreImpl(); }; -/// \brief Forward declaration of metadata. -/// -/// Forward declaration of metadata, in the form of a basic tuple. Unlike \a -/// MDTuple, this class has full support for RAUW, is not owned, is not -/// uniqued, and is suitable for forward references. -class MDNodeFwdDecl : public MDNode { - friend class Metadata; - - MDNodeFwdDecl(LLVMContext &C, ArrayRef Vals) - : MDNode(C, MDNodeFwdDeclKind, Temporary, Vals) {} - -public: - ~MDNodeFwdDecl() { dropAllReferences(); } - - // MSVC doesn't see the alternative: "using MDNode::operator delete". - void operator delete(void *Mem) { MDNode::operator delete(Mem); } - - static MDNodeFwdDecl *get(LLVMContext &Context, ArrayRef MDs) { - return new (MDs.size()) MDNodeFwdDecl(Context, MDs); - } - - static bool classof(const Metadata *MD) { - return MD->getMetadataID() == MDNodeFwdDeclKind; - } -}; - //===----------------------------------------------------------------------===// /// \brief A tuple of MDNodes. /// diff --git a/lib/AsmParser/LLParser.cpp b/lib/AsmParser/LLParser.cpp index 11a5983c8ad..ce195381cfb 100644 --- a/lib/AsmParser/LLParser.cpp +++ b/lib/AsmParser/LLParser.cpp @@ -531,7 +531,7 @@ bool LLParser::ParseMDNodeID(MDNode *&Result) { } // Otherwise, create MDNode forward reference. - MDNodeFwdDecl *FwdNode = MDNodeFwdDecl::get(Context, None); + MDTuple *FwdNode = MDTuple::getTemporary(Context, None); ForwardRefMDNodes[MID] = std::make_pair(FwdNode, Lex.getLoc()); if (NumberedMetadata.size() <= MID) @@ -597,9 +597,9 @@ bool LLParser::ParseStandaloneMetadata() { // See if this was forward referenced, if so, handle it. auto FI = ForwardRefMDNodes.find(MetadataID); if (FI != ForwardRefMDNodes.end()) { - MDNodeFwdDecl *Temp = FI->second.first; + MDTuple *Temp = FI->second.first; Temp->replaceAllUsesWith(Init); - delete Temp; + MDNode::deleteTemporary(Temp); ForwardRefMDNodes.erase(FI); assert(NumberedMetadata[MetadataID] == Init && "Tracking VH didn't work"); diff --git a/lib/AsmParser/LLParser.h b/lib/AsmParser/LLParser.h index 220562d66fc..54e0d5bf020 100644 --- a/lib/AsmParser/LLParser.h +++ b/lib/AsmParser/LLParser.h @@ -136,7 +136,7 @@ namespace llvm { std::vector > NumberedTypes; std::vector NumberedMetadata; - std::map> ForwardRefMDNodes; + std::map> ForwardRefMDNodes; // Global Value reference information. std::map > ForwardRefVals; diff --git a/lib/Bitcode/Reader/BitcodeReader.cpp b/lib/Bitcode/Reader/BitcodeReader.cpp index fe48c254d24..a9adaf4febe 100644 --- a/lib/Bitcode/Reader/BitcodeReader.cpp +++ b/lib/Bitcode/Reader/BitcodeReader.cpp @@ -541,7 +541,7 @@ void BitcodeReaderMDValueList::AssignValue(Metadata *MD, unsigned Idx) { } // If there was a forward reference to this value, replace it. - MDNodeFwdDecl *PrevMD = cast(OldMD.get()); + MDTuple *PrevMD = cast(OldMD.get()); PrevMD->replaceAllUsesWith(MD); MDNode::deleteTemporary(PrevMD); --NumFwdRefs; @@ -573,9 +573,12 @@ void BitcodeReaderMDValueList::tryToResolveCycles() { // Resolve any cycles. for (auto &MD : MDValuePtrs) { - assert(!(MD && isa(MD)) && "Unexpected forward reference"); - if (auto *N = dyn_cast_or_null(MD)) - N->resolveCycles(); + auto *N = dyn_cast_or_null(MD); + if (!N) + continue; + + assert(!N->isTemporary() && "Unexpected forward reference"); + N->resolveCycles(); } } diff --git a/lib/IR/DebugInfo.cpp b/lib/IR/DebugInfo.cpp index 290dbe29c70..c5c11157971 100644 --- a/lib/IR/DebugInfo.cpp +++ b/lib/IR/DebugInfo.cpp @@ -337,7 +337,8 @@ void DIDescriptor::replaceAllUsesWith(LLVMContext &VMContext, DIDescriptor D) { DN = MDNode::get(VMContext, Ops); } - auto *Node = cast(const_cast(DbgNode)); + assert(DbgNode->isTemporary() && "Expected temporary node"); + auto *Node = const_cast(DbgNode); Node->replaceAllUsesWith(const_cast(DN)); MDNode::deleteTemporary(Node); DbgNode = DN; @@ -346,7 +347,8 @@ void DIDescriptor::replaceAllUsesWith(LLVMContext &VMContext, DIDescriptor D) { void DIDescriptor::replaceAllUsesWith(MDNode *D) { assert(DbgNode && "Trying to replace an unverified type!"); assert(DbgNode != D && "This replacement should always happen"); - auto *Node = cast(const_cast(DbgNode)); + assert(DbgNode->isTemporary() && "Expected temporary node"); + auto *Node = const_cast(DbgNode); Node->replaceAllUsesWith(D); MDNode::deleteTemporary(Node); } diff --git a/lib/IR/Metadata.cpp b/lib/IR/Metadata.cpp index 8d83e1652cd..01394137e8e 100644 --- a/lib/IR/Metadata.cpp +++ b/lib/IR/Metadata.cpp @@ -155,7 +155,8 @@ void ReplaceableMetadataImpl::moveRef(void *Ref, void *New, } void ReplaceableMetadataImpl::replaceAllUsesWith(Metadata *MD) { - assert(!(MD && isa(MD)) && "Expected non-temp node"); + assert(!(MD && isa(MD) && cast(MD)->isTemporary()) && + "Expected non-temp node"); if (UseMap.empty()) return; @@ -471,13 +472,14 @@ void UniquableMDNode::resolveCycles() { // Resolve all operands. for (const auto &Op : operands()) { - if (!Op) + auto *N = dyn_cast_or_null(Op); + if (!N) continue; - assert(!isa(Op) && + + assert(!N->isTemporary() && "Expected all forward declarations to be resolved"); - if (auto *N = dyn_cast(Op)) - if (!N->isResolved()) - N->resolveCycles(); + if (!N->isResolved()) + N->resolveCycles(); } } @@ -621,7 +623,7 @@ T *UniquableMDNode::storeImpl(T *N, StorageType Storage, StoreT &Store) { N->storeDistinctInContext(); break; case Temporary: - llvm_unreachable("Unexpected temporary node"); + break; } return N; } @@ -723,13 +725,11 @@ void MDLocation::eraseFromStoreImpl() { getContext().pImpl->MDLocations.erase(this); } -MDNodeFwdDecl *MDNode::getTemporary(LLVMContext &Context, - ArrayRef MDs) { - return MDNodeFwdDecl::get(Context, MDs); +void MDNode::deleteTemporary(MDNode *N) { + assert(N->isTemporary() && "Expected temporary node"); + cast(N)->deleteAsSubclass(); } -void MDNode::deleteTemporary(MDNode *N) { delete cast(N); } - void UniquableMDNode::storeDistinctInContext() { assert(isResolved() && "Expected resolved nodes"); Storage = Distinct; diff --git a/lib/IR/Verifier.cpp b/lib/IR/Verifier.cpp index 4bf2d1a6dc6..f0a149354ae 100644 --- a/lib/IR/Verifier.cpp +++ b/lib/IR/Verifier.cpp @@ -617,7 +617,7 @@ void Verifier::visitMDNode(MDNode &MD) { } // Check these last, so we diagnose problems in operands first. - Assert1(!isa(MD), "Expected no forward declarations!", &MD); + Assert1(!MD.isTemporary(), "Expected no forward declarations!", &MD); Assert1(MD.isResolved(), "All nodes should be resolved!", &MD); } diff --git a/lib/Transforms/Utils/InlineFunction.cpp b/lib/Transforms/Utils/InlineFunction.cpp index 2a86eb598d4..3cebd232984 100644 --- a/lib/Transforms/Utils/InlineFunction.cpp +++ b/lib/Transforms/Utils/InlineFunction.cpp @@ -319,11 +319,11 @@ static void CloneAliasScopeMetadata(CallSite CS, ValueToValueMapTy &VMap) { // Now we have a complete set of all metadata in the chains used to specify // the noalias scopes and the lists of those scopes. - SmallVector DummyNodes; + SmallVector DummyNodes; DenseMap MDMap; for (SetVector::iterator I = MD.begin(), IE = MD.end(); I != IE; ++I) { - MDNode *Dummy = MDNode::getTemporary(CalledFunc->getContext(), None); + MDTuple *Dummy = MDTuple::getTemporary(CalledFunc->getContext(), None); DummyNodes.push_back(Dummy); MDMap[*I].reset(Dummy); } @@ -343,7 +343,8 @@ static void CloneAliasScopeMetadata(CallSite CS, ValueToValueMapTy &VMap) { } MDNode *NewM = MDNode::get(CalledFunc->getContext(), NewOps); - MDNodeFwdDecl *TempM = cast(MDMap[*I]); + MDTuple *TempM = cast(MDMap[*I]); + assert(TempM->isTemporary() && "Expected temporary node"); TempM->replaceAllUsesWith(NewM); } diff --git a/lib/Transforms/Utils/ValueMapper.cpp b/lib/Transforms/Utils/ValueMapper.cpp index 5d89858e527..b195c6349b7 100644 --- a/lib/Transforms/Utils/ValueMapper.cpp +++ b/lib/Transforms/Utils/ValueMapper.cpp @@ -249,12 +249,12 @@ static Metadata *mapDistinctNode(const UniquableMDNode *Node, // In general we need a dummy node, since whether the operands are null can // affect the size of the node. - std::unique_ptr Dummy( - MDNode::getTemporary(Node->getContext(), None)); - mapToMetadata(VM, Node, Dummy.get()); + MDTuple *Dummy = MDTuple::getTemporary(Node->getContext(), None); + mapToMetadata(VM, Node, Dummy); Metadata *NewMD = cloneMDNode(Node, VM, Flags, TypeMapper, Materializer, /* IsDistinct */ true); Dummy->replaceAllUsesWith(NewMD); + MDNode::deleteTemporary(Dummy); return mapToMetadata(VM, Node, NewMD); } @@ -285,7 +285,7 @@ static Metadata *mapUniquedNode(const UniquableMDNode *Node, assert(Node->isUniqued() && "Expected uniqued node"); // Create a dummy node in case we have a metadata cycle. - MDNodeFwdDecl *Dummy = MDNode::getTemporary(Node->getContext(), None); + MDTuple *Dummy = MDTuple::getTemporary(Node->getContext(), None); mapToMetadata(VM, Node, Dummy); // Check all operands to see if any need to be remapped. diff --git a/unittests/IR/MetadataTest.cpp b/unittests/IR/MetadataTest.cpp index 2de1f06aef7..9515f0de4ff 100644 --- a/unittests/IR/MetadataTest.cpp +++ b/unittests/IR/MetadataTest.cpp @@ -167,10 +167,6 @@ TEST_F(MDNodeTest, Delete) { delete I; } -TEST_F(MDNodeTest, DeleteMDNodeFwdDecl) { - delete MDNode::getTemporary(Context, None); -} - TEST_F(MDNodeTest, SelfReference) { // !0 = !{!0} // !1 = !{!0} @@ -343,7 +339,7 @@ TEST_F(MDNodeTest, isTemporary) { TEST_F(MDNodeTest, getDistinctWithUnresolvedOperands) { // temporary !{} - MDNodeFwdDecl *Temp = MDNode::getTemporary(Context, None); + MDTuple *Temp = MDTuple::getTemporary(Context, None); ASSERT_FALSE(Temp->isResolved()); // distinct !{temporary !{}} @@ -364,18 +360,19 @@ TEST_F(MDNodeTest, handleChangedOperandRecursion) { MDNode *N0 = MDNode::get(Context, None); // !1 = !{!3, null} - std::unique_ptr Temp3(MDNode::getTemporary(Context, None)); - Metadata *Ops1[] = {Temp3.get(), nullptr}; + MDTuple *Temp3 = MDTuple::getTemporary(Context, None); + Metadata *Ops1[] = {Temp3, nullptr}; MDNode *N1 = MDNode::get(Context, Ops1); // !2 = !{!3, !0} - Metadata *Ops2[] = {Temp3.get(), N0}; + Metadata *Ops2[] = {Temp3, N0}; MDNode *N2 = MDNode::get(Context, Ops2); // !3 = !{!2} Metadata *Ops3[] = {N2}; MDNode *N3 = MDNode::get(Context, Ops3); Temp3->replaceAllUsesWith(N3); + MDNode::deleteTemporary(Temp3); // !4 = !{!1} Metadata *Ops4[] = {N1}; @@ -428,8 +425,8 @@ TEST_F(MDNodeTest, replaceResolvedOperand) { // a global value that gets RAUW'ed. // // Use a temporary node to keep N from being resolved. - std::unique_ptr Temp(MDNodeFwdDecl::get(Context, None)); - Metadata *Ops[] = {nullptr, Temp.get()}; + MDTuple *Temp = MDTuple::getTemporary(Context, None); + Metadata *Ops[] = {nullptr, Temp}; MDNode *Empty = MDTuple::get(Context, ArrayRef()); MDNode *N = MDTuple::get(Context, Ops); @@ -441,11 +438,12 @@ TEST_F(MDNodeTest, replaceResolvedOperand) { EXPECT_EQ(Empty, N->getOperand(0)); // Check code for adding another unresolved operand. - N->replaceOperandWith(0, Temp.get()); - EXPECT_EQ(Temp.get(), N->getOperand(0)); + N->replaceOperandWith(0, Temp); + EXPECT_EQ(Temp, N->getOperand(0)); // Remove the references to Temp; required for teardown. Temp->replaceAllUsesWith(nullptr); + MDNode::deleteTemporary(Temp); } typedef MetadataTest MDLocationTest; @@ -551,11 +549,11 @@ TEST_F(ValueAsMetadataTest, CollidingDoubleUpdates) { ConstantInt::get(getGlobalContext(), APInt(8, 0))); // Create a temporary to prevent nodes from resolving. - std::unique_ptr Temp(MDNode::getTemporary(Context, None)); + MDTuple *Temp = MDTuple::getTemporary(Context, None); // When the first operand of N1 gets reset to nullptr, it'll collide with N2. - Metadata *Ops1[] = {CI, CI, Temp.get()}; - Metadata *Ops2[] = {nullptr, CI, Temp.get()}; + Metadata *Ops1[] = {CI, CI, Temp}; + Metadata *Ops2[] = {nullptr, CI, Temp}; auto *N1 = MDTuple::get(Context, Ops1); auto *N2 = MDTuple::get(Context, Ops2); @@ -567,10 +565,11 @@ TEST_F(ValueAsMetadataTest, CollidingDoubleUpdates) { ValueAsMetadata::handleDeletion(CI->getValue()); EXPECT_EQ(nullptr, N2->getOperand(0)); EXPECT_EQ(nullptr, N2->getOperand(1)); - EXPECT_EQ(Temp.get(), N2->getOperand(2)); + EXPECT_EQ(Temp, N2->getOperand(2)); // Clean up Temp for teardown. Temp->replaceAllUsesWith(nullptr); + MDNode::deleteTemporary(Temp); } typedef MetadataTest TrackingMDRefTest; -- 2.34.1