Emulate TrackingVH using WeakVH (#6662)

This PR pulls the upstream change, Emulate TrackingVH using WeakVH
(llvm/llvm-project@8a62382),
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

@@ -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<ValueHandleBase**, 2, HandleBaseKind> 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<AssertingVH<T> > {
 #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<typename ValueTy>
-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 <typename ValueTy> 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<ValueTy>(VP) &&
+    assert(isa<ValueTy>(InnerHandle) &&
            "Tracked Value was replaced by one with an invalid type!");
+    return cast<ValueTy>(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<Value*>(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();

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<Value *>::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;
       }

unittests/IR/ValueHandleTest.cpp

@@ -171,6 +171,31 @@ TEST_F(ValueHandle, AssertingVH_ReducesToPointer) {
 
 #else  // !NDEBUG
 
+TEST_F(ValueHandle, TrackingVH_Tracks) {
+  { // HLSL Change
+    TrackingVH<Value> 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<Value> 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<Instruction> 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