Emulate TrackingVH using WeakVH

[sanjoy]

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.

[sanjoy]

FYI unless advised otherwise, I plan to land

https://reviews.llvm.org/D32270
https://reviews.llvm.org/D32268
https://reviews.llvm.org/D32267
https://reviews.llvm.org/D32266

without a re-review once this patch lands.

[chandlerc]

include/llvm/IR/ValueHandle.h

282–283 ↗

(On Diff #97051)

I think we should allow assigning over the TrackingVH as well, much like we would with a moved-from object.

With this change...

289 ↗

(On Diff #97051)

... we can remove the 'null-is-allowed' aspect of the entire class.

Specifically the CheckValidity call can go awoy from setValPtr and instead is only needed for getValPtr.

I don't think this really reduces the utility of the handle in any way and it makes it much simpler IMO.

300 ↗

(On Diff #97051)

And I think this should go back to checking ValueHandleBase::isValid which handles things like trying to extract the value pointer from a densemap key.

[sanjoy]

Address review.

After this TrackingVH is a simple wrapper on WeakVH with two extra checks on access.

[chandlerc]

LGTM, this looks like a nice simplification and frees up the useful bit to support non-tracking VHs. Tiny nit below, but feel free to submit this and the non-tracking weak stuff when you're ready.

include/llvm/IR/ValueHandle.h

314 ↗

(On Diff #97267)

stray space before the '.

[davide]

It looks Chandler reviewed this thoroughly. I have no additional comments.

llvm/trunk/include/llvm/IR/ValueHandle.h

 protected:
   /// \brief This indicates what sub class the handle actually is.
   ///
   /// 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
   };
 
   ValueHandleBase(const ValueHandleBase &RHS)
       : ValueHandleBase(RHS.PrevPair.getInt(), RHS) {}
 
   ValueHandleBase(HandleBaseKind Kind, const ValueHandleBase &RHS)
       : PrevPair(nullptr, Kind), Next(nullptr), Val(RHS.getValPtr()) {
public:
   }
   Value *operator=(const ValueHandleBase &RHS) {
     return ValueHandleBase::operator=(RHS);
   }
 
   operator Value*() const {
     return getValPtr();
   }
+
+  bool pointsToAliveValue() const {
+    return ValueHandleBase::isValid(getValPtr());
+  }
 };
 
 // Specialize simplify_type to allow WeakVH to participate in
 // dyn_cast, isa, etc.
 template <> struct simplify_type<WeakVH> {
   typedef Value *SimpleType;
   static SimpleType getSimplifiedValue(WeakVH &WVH) { return WVH; }
 };

 };
 
 /// \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;
-
-    // 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!");
+///
+/// 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;
+
+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
   // class expects.
   static Value *GetAsValue(Value *V) { return V; }
   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();
   }
 
   ValueTy *operator=(ValueTy *RHS) {
     setValPtr(RHS);
     return getValPtr();

llvm/trunk/lib/IR/Value.cpp

void ValueHandleBase::ValueIsDeleted(Value *V) {
   for (ValueHandleBase Iterator(Assert, *Entry); Entry; Entry = Iterator.Next) {
     Iterator.RemoveFromUseList();
     Iterator.AddToExistingUseListAfter(Entry);
     assert(Entry->Next == &Iterator && "Loop invariant broken.");
 
     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);
       break;
     case Callback:
       // Forward to the subclass's implementation.
       static_cast<CallbackVH*>(Entry)->deleted();
       break;
for (ValueHandleBase Iterator(Assert, *Entry); Entry; Entry = Iterator.Next) {
     Iterator.RemoveFromUseList();
     Iterator.AddToExistingUseListAfter(Entry);
     assert(Entry->Next == &Iterator && "Loop invariant broken.");
 
     switch (Entry->getKind()) {
     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);
       break;
     case Callback:
       // Forward to the subclass's implementation.
       static_cast<CallbackVH*>(Entry)->allUsesReplacedWith(New);
       break;
     }
   }
 
 #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;
       }
 #endif
 }
 
 // Pin the vtable to this file.
 void CallbackVH::anchor() {}

llvm/trunk/unittests/IR/ValueHandleTest.cpp

 #ifdef NDEBUG
 
 TEST_F(ValueHandle, PoisoningVH_ReducesToPointer) {
   EXPECT_EQ(sizeof(CastInst *), sizeof(PoisoningVH<CastInst>));
 }
 
 #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) {
   PoisoningVH<Value> VH(BitcastV.get());
 
   // The poisoned 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, "Accessed a poisoned value handle!");
 
   // Now check that poison catches RAUW.
   VH = BitcastV.get();
   // The replace doesn't trigger anything immediately.
   BitcastV->replaceAllUsesWith(ConstantV);
   // But a use does.
   EXPECT_DEATH((void)*VH, "Accessed a poisoned value handle!");
 
   // 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
 }