From a44c88e2b8032463001c2f379e535a5e16f5807b Mon Sep 17 00:00:00 2001 From: Zhengxing li Date: Tue, 11 Jun 2024 17:49:44 -0700 Subject: [PATCH] Emulate TrackingVH using WeakVH (#6662) This PR pulls the upstream change, Emulate TrackingVH using WeakVH (https://github.com/llvm/llvm-project/commit/8a6238201f015729a47691c62808a23ec8525096), into DXC. Here's the summary of the change: > 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 This is part 2 of the fix for #6659. --- include/llvm/IR/ValueHandle.h | 82 ++++++++++++++++++++------------ lib/IR/Value.cpp | 19 ++------ unittests/IR/ValueHandleTest.cpp | 33 +++++++++++++ 3 files changed, 87 insertions(+), 47 deletions(-) diff --git a/include/llvm/IR/ValueHandle.h b/include/llvm/IR/ValueHandle.h index 08a59806a2..345f9dcaf5 100644 --- a/include/llvm/IR/ValueHandle.h +++ b/include/llvm/IR/ValueHandle.h @@ -45,12 +45,7 @@ class ValueHandleBase { /// /// This is to avoid having a vtable for the light-weight handle pointers. The /// fully general Callback version does have a vtable. - enum HandleBaseKind { - Assert, - Callback, - Tracking, - Weak - }; + enum HandleBaseKind { Assert, Callback, Weak }; private: PointerIntPair PrevPair; @@ -165,6 +160,10 @@ class WeakVH : public ValueHandleBase { operator Value*() const { return getValPtr(); } + + bool pointsToAliveValue() const { + return ValueHandleBase::isValid(getValPtr()); + } }; // Specialize simplify_type to allow WeakVH to participate in @@ -275,46 +274,65 @@ struct isPodLike > { #endif }; - /// \brief Value handle that tracks a Value across RAUW. /// /// TrackingVH is designed for situations where a client needs to hold a handle /// 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(); - - // Null is always ok. - if (!VP) return; +/// +/// 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; - // 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 { + // HLSL Change begin + // The original upstream change will assert here when accessing a TrackingVH + // is deleted. + // + // However, the llvm code that DXC forked has the implicit code like: + // TrackingVH V = nullptr; + // + // It will invoke setValPtr(nullptr) and then getValPtr(nullptr). So pull in + // the original upstream change in DXC will always assert here for debug + // build even this code is valid. + // + // The original upstream change works because of another upstream change + // https://github.com/llvm/llvm-project/commit/70a6051ddfd5f04777f2bc42503bb11bc8f1723a + // cleaned up the problematic code in DXC already. + // + // Untill we decide to pull that upstream change into DXC, DXC should follow + // the original TrackingVH implementation. return Null is always ok here + // instead of assert it. + if (InnerHandle.operator llvm::Value *() == nullptr) + return nullptr; + // HLSL Change end. + + 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 @@ -323,9 +341,11 @@ 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(const TrackingVH &RHS) : ValueHandleBase(Tracking, RHS) {} + TrackingVH() {} + TrackingVH(ValueTy *P) { setValPtr(P); } + TrackingVH(const TrackingVH &RHS) { + setValPtr(RHS.getValPtr()); + } // HLSL Change operator ValueTy*() const { return getValPtr(); diff --git a/lib/IR/Value.cpp b/lib/IR/Value.cpp index 0b07147694..3fc7f094b4 100644 --- a/lib/IR/Value.cpp +++ b/lib/IR/Value.cpp @@ -672,11 +672,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); @@ -729,13 +724,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; // HLSL CHANGE case Weak: // Weak goes to the new value, which will unlink it from Old's list. Entry->operator=(New); @@ -748,18 +736,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 e6eb7cbedc..51de428531 100644 --- a/unittests/IR/ValueHandleTest.cpp +++ b/unittests/IR/ValueHandleTest.cpp @@ -171,6 +171,31 @@ TEST_F(ValueHandle, AssertingVH_ReducesToPointer) { #else // !NDEBUG +TEST_F(ValueHandle, TrackingVH_Tracks) { + { // HLSL Change + TrackingVH VH(BitcastV.get()); + BitcastV->replaceAllUsesWith(ConstantV); + EXPECT_EQ(VH, ConstantV); + } // HLSL Change + + // HLSL Change begin + // This test is a DEATH_TEST in the original upstream change. It will + // assert when accessing a TrackingVH is deleted. + // However, DXC should follow the original TrackingVH implementation. + // return Null is always ok instead of assert it. + // Check the comment in TrackingVH::getValPtr() for more detail. + { + TrackingVH VH(BitcastV.get()); + + // The tracking handle shouldn't assert when the value is deleted. + BitcastV.reset( + new BitCastInst(ConstantV, Type::getInt32Ty(getGlobalContext()))); + // The handle should be nullptr after it's deleted. + EXPECT_EQ(VH, nullptr); + } + // HLSL Change end +} + #ifdef GTEST_HAS_DEATH_TEST TEST_F(ValueHandle, AssertingVH_Asserts) { @@ -185,6 +210,14 @@ TEST_F(ValueHandle, AssertingVH_Asserts) { BitcastV.reset(); } +TEST_F(ValueHandle, TrackingVH_Asserts) { + 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