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

@@ -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<AssertingVH<T> > {
 /// 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();
+///
+/// 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;
 
-    // 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<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
@@ -321,8 +322,8 @@ 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() {}
+  TrackingVH(ValueTy *P) { setValPtr(P); }
 
   operator ValueTy*() const {
     return getValPtr();

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

unittests/IR/ValueHandleTest.cpp

@@ -482,6 +482,12 @@ TEST_F(ValueHandle, PoisoningVH_ReducesToPointer) {
 
 #else // !NDEBUG
 
+TEST_F(ValueHandle, TrackingVH_Tracks) {
+  TrackingVH<Value> 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<Value> 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<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