[NewGVN] Simplify eliminateInstructions

llvm/lib/Transforms/Scalar/NewGVN.cpp

@@ -3564,11 +3564,9 @@ struct NewGVN::ValueDFS {
   int DFSOut = 0;
   int LocalNum = 0;
 
-  // Only one of Def and U will be set.
   // The bool in the Def tells us whether the Def is the stored value of a
   // store.
   PointerIntPair<Value *, 1, bool> Def;
-  Use *U = nullptr;
 
   bool operator<(const ValueDFS &Other) const {
     // It's not enough that any given field be less than - we have sets
@@ -3585,33 +3583,8 @@ struct NewGVN::ValueDFS {
     // Each LLVM instruction only produces one value, and thus the lowest-level
     // differentiator that really matters for the stack (and what we use as a
     // replacement) is the local dfs number.
-    // Everything else in the structure is instruction level, and only affects
-    // the order in which we will replace operands of a given instruction.
-    //
-    // For a given instruction (IE things with equal dfsin, dfsout, localnum),
-    // the order of replacement of uses does not matter.
-    // IE given,
-    //  a = 5
-    //  b = a + a
-    // When you hit b, you will have two valuedfs with the same dfsin, out, and
-    // localnum.
-    // The .val will be the same as well.
-    // The .u's will be different.
-    // You will replace both, and it does not matter what order you replace them
-    // in (IE whether you replace operand 2, then operand 1, or operand 1, then
-    // operand 2).
-    // Similarly for the case of same dfsin, dfsout, localnum, but different
-    // .val's
-    //  a = 5
-    //  b  = 6
-    //  c = a + b
-    // in c, we will a valuedfs for a, and one for b,with everything the same
-    // but .val  and .u.
-    // It does not matter what order we replace these operands in.
-    // You will always end up with the same IR, and this is guaranteed.
-    return std::tie(DFSIn, DFSOut, LocalNum, Def, U) <
-           std::tie(Other.DFSIn, Other.DFSOut, Other.LocalNum, Other.Def,
-                    Other.U);
+    return std::tie(DFSIn, DFSOut, LocalNum, Def) <
+           std::tie(Other.DFSIn, Other.DFSOut, Other.LocalNum, Other.Def);
   }
 };
 
@@ -3672,30 +3645,14 @@ void NewGVN::convertClassToDFSOrdered(
         // Don't try to replace into dead uses
         if (InstructionsToErase.count(I))
           continue;
-        ValueDFS VDUse;
         // Put the phi node uses in the incoming block.
-        BasicBlock *IBlock;
-        if (auto *P = dyn_cast<PHINode>(I)) {
-          IBlock = P->getIncomingBlock(U);
-          // Make phi node users appear last in the incoming block
-          // they are from.
-          VDUse.LocalNum = InstrDFS.size() + 1;
-        } else {
-          IBlock = getBlockForValue(I);
-          VDUse.LocalNum = InstrToDFSNum(I);
-        }
-
+        auto *PN = dyn_cast<PHINode>(I);
+        BasicBlock *IBlock = PN ? PN->getIncomingBlock(U) : getBlockForValue(I);
         // Skip uses in unreachable blocks, as we're going
         // to delete them.
         if (!ReachableBlocks.contains(IBlock))
           continue;
-
-        DomTreeNode *DomNode = DT->getNode(IBlock);
-        VDUse.DFSIn = DomNode->getDFSNumIn();
-        VDUse.DFSOut = DomNode->getDFSNumOut();
-        VDUse.U = &U;
         ++UseCount;
-        DFSOrderedSet.emplace_back(VDUse);
       }
     }
 
@@ -4004,7 +3961,6 @@ bool NewGVN::eliminateInstructions(Function &F) {
           int MemberDFSOut = VD.DFSOut;
           Value *Def = VD.Def.getPointer();
           bool FromStore = VD.Def.getInt();
-          Use *U = VD.U;
           // We ignore void things because we can't get a value from them.
           if (Def && Def->getType()->isVoidTy())
             continue;
@@ -4061,101 +4017,48 @@ bool NewGVN::eliminateInstructions(Function &F) {
             }
           }
 
-          // Skip the Def's, we only want to eliminate on their uses.  But mark
-          // dominated defs as dead.
-          if (Def) {
-            // For anything in this case, what and how we value number
-            // guarantees that any side-effects that would have occurred (ie
-            // throwing, etc) can be proven to either still occur (because it's
-            // dominated by something that has the same side-effects), or never
-            // occur.  Otherwise, we would not have been able to prove it value
-            // equivalent to something else. For these things, we can just mark
-            // it all dead.  Note that this is different from the "ProbablyDead"
-            // set, which may not be dominated by anything, and thus, are only
-            // easy to prove dead if they are also side-effect free. Note that
-            // because stores are put in terms of the stored value, we skip
-            // stored values here. If the stored value is really dead, it will
-            // still be marked for deletion when we process it in its own class.
-            auto *DefI = dyn_cast<Instruction>(Def);
-            if (!EliminationStack.empty() && DefI && !FromStore) {
-              Value *DominatingLeader = EliminationStack.back();
-              if (DominatingLeader != Def) {
+          // For anything in this case, what and how we value number
+          // guarantees that any side-effects that would have occurred (ie
+          // throwing, etc) can be proven to either still occur (because it's
+          // dominated by something that has the same side-effects), or never
+          // occur.  Otherwise, we would not have been able to prove it value
+          // equivalent to something else. For these things, we can just mark
+          // it all dead.  Note that this is different from the "ProbablyDead"
+          // set, which may not be dominated by anything, and thus, are only
+          // easy to prove dead if they are also side-effect free. Note that
+          // because stores are put in terms of the stored value, we skip
+          // stored values here. If the stored value is really dead, it will
+          // still be marked for deletion when we process it in its own class.
+          auto *DefI = dyn_cast<Instruction>(Def);
+          if (!EliminationStack.empty() && DefI && !FromStore) {
+            Value *DominatingLeader = EliminationStack.back();
+            auto *II = dyn_cast<IntrinsicInst>(DominatingLeader);
+            bool isSSACopy = II && II->getIntrinsicID() == Intrinsic::ssa_copy;
+            if (isSSACopy)
+              DominatingLeader = II->getOperand(0);
+            if (DominatingLeader != Def) {
+              // All uses of Def are now uses of the dominating leader, which
+              // means if the dominating leader was dead, it's now live!
+              auto &LeaderUseCount = UseCounts[DominatingLeader];
+              if (LeaderUseCount == 0 && isa<Instruction>(DominatingLeader))
+                ProbablyDead.erase(cast<Instruction>(DominatingLeader));
+              LeaderUseCount += Def->getNumUses();
+
+              // Don't patch metadata if ssa_copy is being replaced by their
+              // original.
+              auto *PI = PredInfo->getPredicateInfoFor(DefI);
+              if (PI && DominatingLeader == PI->OriginalOp) {
+                Def->replaceAllUsesWith(DominatingLeader);
+              } else {
                 // Even if the instruction is removed, we still need to update
                 // flags/metadata due to downstreams users of the leader.
-                if (!match(DefI, m_Intrinsic<Intrinsic::ssa_copy>()))
-                  patchReplacementInstruction(DefI, DominatingLeader);
-
-                markInstructionForDeletion(DefI);
+                patchAndReplaceAllUsesWith(cast<Instruction>(Def),
+                                           DominatingLeader);
               }
-            }
-            continue;
-          }
-          // At this point, we know it is a Use we are trying to possibly
-          // replace.
-
-          assert(isa<Instruction>(U->get()) &&
-                 "Current def should have been an instruction");
-          assert(isa<Instruction>(U->getUser()) &&
-                 "Current user should have been an instruction");
-
-          // If the thing we are replacing into is already marked to be dead,
-          // this use is dead.  Note that this is true regardless of whether
-          // we have anything dominating the use or not.  We do this here
-          // because we are already walking all the uses anyway.
-          Instruction *InstUse = cast<Instruction>(U->getUser());
-          if (InstructionsToErase.count(InstUse)) {
-            auto &UseCount = UseCounts[U->get()];
-            if (--UseCount == 0) {
-              ProbablyDead.insert(cast<Instruction>(U->get()));
-            }
-          }
-
-          // If we get to this point, and the stack is empty we must have a use
-          // with nothing we can use to eliminate this use, so just skip it.
-          if (EliminationStack.empty())
-            continue;
-
-          Value *DominatingLeader = EliminationStack.back();
-
-          auto *II = dyn_cast<IntrinsicInst>(DominatingLeader);
-          bool isSSACopy = II && II->getIntrinsicID() == Intrinsic::ssa_copy;
-          if (isSSACopy)
-            DominatingLeader = II->getOperand(0);
-
-          // Don't replace our existing users with ourselves.
-          if (U->get() == DominatingLeader)
-            continue;
-
-          // If we replaced something in an instruction, handle the patching of
-          // metadata.  Skip this if we are replacing predicateinfo with its
-          // original operand, as we already know we can just drop it.
-          auto *ReplacedInst = cast<Instruction>(U->get());
-          auto *PI = PredInfo->getPredicateInfoFor(ReplacedInst);
-          if (!PI || DominatingLeader != PI->OriginalOp)
-            patchReplacementInstruction(ReplacedInst, DominatingLeader);
-
-          LLVM_DEBUG(dbgs()
-                     << "Found replacement " << *DominatingLeader << " for "
-                     << *U->get() << " in " << *(U->getUser()) << "\n");
-          U->set(DominatingLeader);
-          // This is now a use of the dominating leader, which means if the
-          // dominating leader was dead, it's now live!
-          auto &LeaderUseCount = UseCounts[DominatingLeader];
-          // It's about to be alive again.
-          if (LeaderUseCount == 0 && isa<Instruction>(DominatingLeader))
-            ProbablyDead.erase(cast<Instruction>(DominatingLeader));
-          // For copy instructions, we use their operand as a leader,
-          // which means we remove a user of the copy and it may become dead.
-          if (isSSACopy) {
-            auto It = UseCounts.find(II);
-            if (It != UseCounts.end()) {
-              unsigned &IIUseCount = It->second;
-              if (--IIUseCount == 0)
-                ProbablyDead.insert(II);
+              AnythingReplaced = true;
+              markInstructionForDeletion(DefI);
             }
           }
-          ++LeaderUseCount;
-          AnythingReplaced = true;
         }
       }
     }

llvm/test/Transforms/NewGVN/commute.ll

@@ -48,6 +48,7 @@ declare i16 @llvm.umul.fix.i16(i16, i16, i32)
 
 define i16 @intrinsic_3_args(i16 %x, i16 %y) {
 ; CHECK-LABEL: @intrinsic_3_args(
+; CHECK-NEXT:    [[M1:%.*]] = call i16 @llvm.smul.fix.i16(i16 [[X:%.*]], i16 [[Y:%.*]], i32 1)
 ; CHECK-NEXT:    ret i16 0
 ;
   %m1 = call i16 @llvm.smul.fix.i16(i16 %x, i16 %y, i32 1)

llvm/test/Transforms/NewGVN/invariant.group.ll

@@ -74,6 +74,7 @@ entry:
 define i1 @proveEqualityForStrip(ptr %a) {
 ; CHECK-LABEL: define i1 @proveEqualityForStrip(
 ; CHECK-SAME: ptr [[A:%.*]]) {
+; CHECK-NEXT:    [[B1:%.*]] = call ptr @llvm.strip.invariant.group.p0(ptr [[A]])
 ; CHECK-NEXT:    ret i1 true
 ;
   %b1 = call ptr @llvm.strip.invariant.group.p0(ptr %a)

llvm/test/Transforms/NewGVN/load-constant-mem.ll

@@ -7,6 +7,7 @@ define i32 @test(ptr %p, i32 %i) nounwind {
 ; CHECK-LABEL: @test(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[P:%.*]] = getelementptr [4 x i32], ptr @G, i32 0, i32 [[I:%.*]]
+; CHECK-NEXT:    [[A:%.*]] = load i32, ptr [[P]], align 4
 ; CHECK-NEXT:    store i8 4, ptr [[P1:%.*]], align 1
 ; CHECK-NEXT:    ret i32 0
 ;

llvm/test/Transforms/NewGVN/pr31682.ll

@@ -12,6 +12,7 @@ define void @bar(i1 %arg) {
 ; CHECK-NEXT:    [[TMP:%.*]] = load ptr, ptr @global, align 8
 ; CHECK-NEXT:    br label [[BB2:%.*]]
 ; CHECK:       bb2:
+; CHECK-NEXT:    [[TMP4:%.*]] = getelementptr [[STRUCT_FOO:%.*]], ptr [[TMP]], i64 0, i32 1
 ; CHECK-NEXT:    br i1 %arg, label [[BB2]], label [[BB7:%.*]]
 ; CHECK:       bb7:
 ; CHECK-NEXT:    br label [[BB10:%.*]]

llvm/test/Transforms/NewGVN/tbaa.ll

@@ -95,6 +95,7 @@ define i32 @test7(ptr %p, ptr %q) {
 define i32 @test8(ptr %p, ptr %q) {
 ; CHECK-LABEL: define i32 @test8(
 ; CHECK-SAME: ptr [[P:%.*]], ptr [[Q:%.*]]) {
+; CHECK-NEXT:    [[A:%.*]] = load i32, ptr [[Q]], align 4, !tbaa [[TBAA7:![0-9]+]]
 ; CHECK-NEXT:    store i32 15, ptr [[P]], align 4
 ; CHECK-NEXT:    ret i32 0
 ;
@@ -111,6 +112,7 @@ define i32 @test8(ptr %p, ptr %q) {
 define i32 @test9(ptr %p, ptr %q) {
 ; CHECK-LABEL: define i32 @test9(
 ; CHECK-SAME: ptr [[P:%.*]], ptr [[Q:%.*]]) {
+; CHECK-NEXT:    [[A:%.*]] = load i32, ptr [[Q]], align 4, !tbaa [[TBAA7]]
 ; CHECK-NEXT:    call void @clobber()
 ; CHECK-NEXT:    ret i32 0
 ;
@@ -172,9 +174,9 @@ declare i32 @foo(ptr) readonly
 ; CHECK: [[TBAA4]] = !{[[META5:![0-9]+]], [[META5]], i64 0}
 ; CHECK: [[META5]] = !{!"B", [[META2]]}
 ; CHECK: [[TBAA6]] = !{[[META2]], [[META2]], i64 0}
-; CHECK: [[TBAA7]] = !{[[META8:![0-9]+]], [[META8]], i64 0}
-; CHECK: [[META8]] = !{!"scalar type", [[META9:![0-9]+]]}
-; CHECK: [[META9]] = !{!"another root"}
+; CHECK: [[TBAA7]] = !{[[META8:![0-9]+]], [[META8]], i64 0, i64 1}
+; CHECK: [[META8]] = !{!"node", [[META9:![0-9]+]]}
+; CHECK: [[META9]] = !{!"yet another root"}
 ; CHECK: [[TBAA10]] = !{[[META11:![0-9]+]], [[META12:![0-9]+]], i64 0}
 ; CHECK: [[META11]] = !{!"struct X", [[META12]], i64 0}
 ; CHECK: [[META12]] = !{!"int", [[META13:![0-9]+]], i64 0}