[LoopUnroll] Avoid UB when converting from WeakVH to `Value *`

[bogner]

Summary

Calling operator* on a WeakVH with a null value yields a null
reference, which is UB. Avoid this by implicitly converting the WeakVH
to a Value * rather than dereferencing and then taking the address
for the type conversion.

llvm/include/llvm/IR/ValueHandle.h

if (isValid(getValPtr()))
       RemoveFromUseList();
     setValPtr(RHS.getValPtr());
     if (isValid(getValPtr()))
       AddToExistingUseList(RHS.getPrevPtr());
     return getValPtr();
   }
 
   Value *operator->() const { return getValPtr(); }
-  Value &operator*() const { return *getValPtr(); }
+  Value &operator*() const {
+    Value *V = getValPtr();
+    assert(V && "Dereferencing deleted ValueHandle");
+    return *V;
+  }
 
 protected:
   Value *getValPtr() const { return Val; }
 
   static bool isValid(Value *V) {
     return V &&
            V != DenseMapInfo<Value *>::getEmptyKey() &&
            V != DenseMapInfo<Value *>::getTombstoneKey();

llvm/lib/Transforms/Utils/LoopUnroll.cpp

void llvm::simplifyLoopAfterUnroll(Loop *L, bool SimplifyIVs, LoopInfo *LI,
                                    AssumptionCache *AC) {
   // Simplify any new induction variables in the partially unrolled loop.
   if (SE && SimplifyIVs) {
     SmallVector<WeakTrackingVH, 16> DeadInsts;
     simplifyLoopIVs(L, SE, DT, LI, DeadInsts);
 
     // Aggressively clean up dead instructions that simplifyLoopIVs already
     // identified. Any remaining should be cleaned up below.
-    while (!DeadInsts.empty())
-      if (Instruction *Inst =
-              dyn_cast_or_null<Instruction>(&*DeadInsts.pop_back_val()))
+    while (!DeadInsts.empty()) {
+      Value *V = DeadInsts.pop_back_val();
+      if (Instruction *Inst = dyn_cast_or_null<Instruction>(V))
         RecursivelyDeleteTriviallyDeadInstructions(Inst);
     }
+  }
 
   // At this point, the code is well formed.  We now do a quick sweep over the
   // inserted code, doing constant propagation and dead code elimination as we
   // go.
   const DataLayout &DL = L->getHeader()->getModule()->getDataLayout();
   for (BasicBlock *BB : L->getBlocks()) {
     for (BasicBlock::iterator I = BB->begin(), E = BB->end(); I != E;) {
       Instruction *Inst = &*I++;

llvm/test/Transforms/LoopUnroll/partial-unroll-dead-instructions.ll

+; RUN: opt -S < %s -loop-unroll -unroll-allow-partial=1 | FileCheck %s
+;
+; Bugpointed test that triggered UB while cleaning up dead
+; instructions after simplifying indvars
+
+; We just check that some unrolling happened here - the assert we've
+; added to ValueHandleBase::operator* would fire if the bug was still
+; present.
+; CHECK: atomicrmw volatile add i32*
+; CHECK: atomicrmw volatile add i32*
+; CHECK: atomicrmw volatile add i32*
+
+@global = external global i32, align 4
+
+define void @widget() {
+bb:
+  br label %bb1
+
+bb1:
+  br label %bb2
+
+bb2:
+  %tmp = phi i32 [ 0, %bb1 ], [ %tmp34, %bb33 ]
+  %tmp3 = phi i32 [ 0, %bb1 ], [ %tmp34, %bb33 ]
+  %tmp26 = and i32 %tmp, 1073741823
+  %tmp27 = getelementptr inbounds i32, i32* @global, i32 %tmp26
+  %tmp28 = atomicrmw volatile add i32* %tmp27, i32 1 monotonic
+  %tmp29 = icmp ugt i32 %tmp28, 23
+  %tmp30 = shl i32 %tmp, 6
+  %tmp31 = add i32 %tmp30, undef
+  %tmp32 = add i32 %tmp31, %tmp28
+  store i32 undef, i32* undef, align 4
+  br label %bb33
+
+bb33:
+  %tmp34 = add nuw nsw i32 %tmp3, 1
+  %tmp35 = icmp ult i32 %tmp3, 15
+  br i1 %tmp35, label %bb2, label %bb36
+
+bb36:
+  br label %bb1
+}