From faa5631a32baa277148524c7a4ae792b62304e74 Mon Sep 17 00:00:00 2001 From: Sanjoy Das Date: Mon, 1 May 2017 16:28:58 +0000 Subject: [PATCH] Emulate TrackingVH using WeakVH Summary: This frees up one slot in the HandleBaseKind enum, which I will use later to add a new kind of value handle. The size of the HandleBaseKind enum is important because we store a HandleBaseKind in the low two bits of a (in the worst case) 4 byte aligned pointer. Reviewers: davide, chandlerc Subscribers: mcrosier, llvm-commits Differential Revision: https://reviews.llvm.org/D32634 llvm-svn: 301809 --- include/llvm/IR/ValueHandle.h | 49 ++++++++++++++++---------------- lib/IR/Value.cpp | 19 ++----------- unittests/IR/ValueHandleTest.cpp | 26 +++++++++++++++++ 3 files changed, 54 insertions(+), 40 deletions(-) diff --git a/include/llvm/IR/ValueHandle.h b/include/llvm/IR/ValueHandle.h index 4932d0143ce..17b65c6511c 100644 --- a/include/llvm/IR/ValueHandle.h +++ b/include/llvm/IR/ValueHandle.h @@ -37,7 +37,6 @@ protected: enum HandleBaseKind { Assert, Callback, - Tracking, Weak }; @@ -165,6 +164,10 @@ public: operator Value*() const { return getValPtr(); } + + bool pointsToAliveValue() const { + return ValueHandleBase::isValid(getValPtr()); + } }; // Specialize simplify_type to allow WeakVH to participate in @@ -280,39 +283,37 @@ struct isPodLike > { /// to a Value (or subclass) across some operations which may move that value, /// but should never destroy it or replace it with some unacceptable type. /// -/// It is an error to do anything with a TrackingVH whose value has been -/// destroyed, except to destruct it. -/// /// It is an error to attempt to replace a value with one of a type which is /// incompatible with any of its outstanding TrackingVHs. -template -class TrackingVH : public ValueHandleBase { - void CheckValidity() const { - Value *VP = ValueHandleBase::getValPtr(); +/// +/// It is an error to read from a TrackingVH that does not point to a valid +/// value. A TrackingVH is said to not point to a valid value if either it +/// hasn't yet been assigned a value yet or because the value it was tracking +/// has since been deleted. +/// +/// Assigning a value to a TrackingVH is always allowed, even if said TrackingVH +/// no longer points to a valid value. +template class TrackingVH { + WeakVH InnerHandle; - // Null is always ok. - if (!VP) return; - - // Check that this value is valid (i.e., it hasn't been deleted). We - // explicitly delay this check until access to avoid requiring clients to be - // unnecessarily careful w.r.t. destruction. - assert(ValueHandleBase::isValid(VP) && "Tracked Value was deleted!"); +public: + ValueTy *getValPtr() const { + assert(InnerHandle.pointsToAliveValue() && + "TrackingVH must be non-null and valid on dereference!"); // Check that the value is a member of the correct subclass. We would like // to check this property on assignment for better debugging, but we don't // want to require a virtual interface on this VH. Instead we allow RAUW to // replace this value with a value of an invalid type, and check it here. - assert(isa(VP) && + assert(isa(InnerHandle) && "Tracked Value was replaced by one with an invalid type!"); + return cast(InnerHandle); } - ValueTy *getValPtr() const { - CheckValidity(); - return (ValueTy*)ValueHandleBase::getValPtr(); - } void setValPtr(ValueTy *P) { - CheckValidity(); - ValueHandleBase::operator=(GetAsValue(P)); + // Assigning to non-valid TrackingVH's are fine so we just unconditionally + // assign here. + InnerHandle = GetAsValue(P); } // Convert a ValueTy*, which may be const, to the type the base @@ -321,8 +322,8 @@ class TrackingVH : public ValueHandleBase { static Value *GetAsValue(const Value *V) { return const_cast(V); } public: - TrackingVH() : ValueHandleBase(Tracking) {} - TrackingVH(ValueTy *P) : ValueHandleBase(Tracking, GetAsValue(P)) {} + TrackingVH() {} + TrackingVH(ValueTy *P) { setValPtr(P); } operator ValueTy*() const { return getValPtr(); diff --git a/lib/IR/Value.cpp b/lib/IR/Value.cpp index 906c5d33535..2ce0a2ecf2b 100644 --- a/lib/IR/Value.cpp +++ b/lib/IR/Value.cpp @@ -820,11 +820,6 @@ void ValueHandleBase::ValueIsDeleted(Value *V) { switch (Entry->getKind()) { case Assert: break; - case Tracking: - // Mark that this value has been deleted by setting it to an invalid Value - // pointer. - Entry->operator=(DenseMapInfo::getTombstoneKey()); - break; case Weak: // Weak just goes to null, which will unlink it from the list. Entry->operator=(nullptr); @@ -876,13 +871,6 @@ void ValueHandleBase::ValueIsRAUWd(Value *Old, Value *New) { case Assert: // Asserting handle does not follow RAUW implicitly. break; - case Tracking: - // Tracking goes to new value like a WeakVH. Note that this may make it - // something incompatible with its templated type. We don't want to have a - // virtual (or inline) interface to handle this though, so instead we make - // the TrackingVH accessors guarantee that a client never sees this value. - - LLVM_FALLTHROUGH; case Weak: // Weak goes to the new value, which will unlink it from Old's list. Entry->operator=(New); @@ -895,18 +883,17 @@ void ValueHandleBase::ValueIsRAUWd(Value *Old, Value *New) { } #ifndef NDEBUG - // If any new tracking or weak value handles were added while processing the + // If any new weak value handles were added while processing the // list, then complain about it now. if (Old->HasValueHandle) for (Entry = pImpl->ValueHandles[Old]; Entry; Entry = Entry->Next) switch (Entry->getKind()) { - case Tracking: case Weak: dbgs() << "After RAUW from " << *Old->getType() << " %" << Old->getName() << " to " << *New->getType() << " %" << New->getName() << "\n"; - llvm_unreachable("A tracking or weak value handle still pointed to the" - " old value!\n"); + llvm_unreachable( + "A weak value handle still pointed to the old value!\n"); default: break; } diff --git a/unittests/IR/ValueHandleTest.cpp b/unittests/IR/ValueHandleTest.cpp index 1abc87c2fdc..e19b3a639d3 100644 --- a/unittests/IR/ValueHandleTest.cpp +++ b/unittests/IR/ValueHandleTest.cpp @@ -482,6 +482,12 @@ TEST_F(ValueHandle, PoisoningVH_ReducesToPointer) { #else // !NDEBUG +TEST_F(ValueHandle, TrackingVH_Tracks) { + TrackingVH VH(BitcastV.get()); + BitcastV->replaceAllUsesWith(ConstantV); + EXPECT_EQ(VH, ConstantV); +} + #ifdef GTEST_HAS_DEATH_TEST TEST_F(ValueHandle, PoisoningVH_Asserts) { @@ -502,6 +508,26 @@ TEST_F(ValueHandle, PoisoningVH_Asserts) { // Don't clear anything out here as destroying the handles should be fine. } +TEST_F(ValueHandle, TrackingVH_Asserts) { + { + TrackingVH VH(BitcastV.get()); + + // The tracking handle shouldn't assert when the value is deleted. + BitcastV.reset(new BitCastInst(ConstantV, Type::getInt32Ty(Context))); + // But should when we access the handle. + EXPECT_DEATH((void)*VH, + "TrackingVH must be non-null and valid on dereference!"); + } + + { + TrackingVH VH(BitcastV.get()); + + BitcastV->replaceAllUsesWith(ConstantV); + EXPECT_DEATH((void)*VH, + "Tracked Value was replaced by one with an invalid type!"); + } +} + #endif // GTEST_HAS_DEATH_TEST #endif // NDEBUG