[RemoveDIs] Handle DPValues in hoistCommonCodeFromSuccessors

[OCHyams]

[RemoveDIs] Handle DPValues in hoistCommonCodeFromSuccessors

Hoist DPValues attached to each instruction being considered for hoisting if they
are identical in lock-step. This includes the final instructions which are
considered but not hoisted, because the corresponding dbg.values would appear
before those instruction and thus hoisted if identical.

Identical debug records hoisted:
llvm/test/Transforms/SimplifyCFG/hoist-dbgvalue.ll

Non-identical debug records not hoisted:
llvm/test/Transforms/SimplifyCFG/X86/pr39187-g.ll

Debug records attached to first not-hoisted instructions are hoisted:
llvm/test/Transforms/SimplifyCFG/hoist-dbgvalue-inlined.ll

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Orlando Cazalet-Hyams (OCHyams)

Changes

[RemoveDIs] Handle DPValues in hoistCommonCodeFromSuccessors

Hoist DPValues attached to each instruction being considered for hoisting if they
are identical in lock-step. This includes the final instructions which are
considered but not hoisted, because the corresponding dbg.values would appear
before those instruction and thus hoisted if identical.

Identical debug records hoisted:
llvm/test/Transforms/SimplifyCFG/hoist-dbgvalue.ll

Non-identical debug records not hoisted:
llvm/test/Transforms/SimplifyCFG/X86/pr39187-g.ll

Debug records attached to first not-hoisted instructions are hoisted:
llvm/test/Transforms/SimplifyCFG/hoist-dbgvalue-inlined.ll

---

Full diff: https://github.com/llvm/llvm-project/pull/79476.diff

4 Files Affected:

• (modified) llvm/lib/Transforms/Utils/SimplifyCFG.cpp (+87-12)
• (modified) llvm/test/Transforms/SimplifyCFG/X86/pr39187-g.ll (+1)
• (modified) llvm/test/Transforms/SimplifyCFG/hoist-dbgvalue-inlined.ll (+1)
• (modified) llvm/test/Transforms/SimplifyCFG/hoist-dbgvalue.ll (+1)

diff --git a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
index f3994b6cc39fefb..45bf4483f0d62e7 100644
--- a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
+++ b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
@@ -1526,6 +1526,63 @@ static bool shouldHoistCommonInstructions(Instruction *I1, Instruction *I2,
   return true;
 }
 
+/// Hoists DPValues from \p I1 and \p OtherInstrs that are identical in
+/// lock-step to \p TI. This matches how dbg.* intrinsics are hoisting in
+/// hoistCommonCodeFromSuccessors. e.g. The input:
+///    I1                DPVs: { x, z },
+///    OtherInsts: { I2  DPVs: { x, y, z } }
+/// would result in hoisting only DPValue x.
+static void
+hoistLockstepIdenticalDPValues(Instruction *TI, Instruction *I1,
+                               SmallVectorImpl<Instruction *> &OtherInsts) {
+  if (!I1->hasDbgValues())
+    return;
+  using CurrentAndEndIt =
+      std::pair<DPValue::self_iterator, DPValue::self_iterator>;
+  // Vector of {Current, End} iterators.
+  SmallVector<CurrentAndEndIt> Itrs;
+  Itrs.reserve(OtherInsts.size() + 1);
+  // Helper lambdas for lock-step checks:
+  // Return true if any Current == End.
+  auto atEnd = [&]() {
+    return any_of(Itrs,
+                  [](const CurrentAndEndIt &P) { return P.first == P.second; });
+  };
+  // Return true if all Current are identical.
+  auto allIdentical = [&]() {
+    return all_of(make_first_range(ArrayRef(Itrs).drop_front()),
+                  [&](DPValue::self_iterator I) {
+                    return Itrs[0].first->isIdenticalToWhenDefined(*I);
+                  });
+  };
+
+  // Collect the iterators.
+  Itrs.push_back(
+      {I1->getDbgValueRange().begin(), I1->getDbgValueRange().end()});
+  for (Instruction *Other : OtherInsts) {
+    if (!Other->hasDbgValues())
+      return;
+    Itrs.push_back(
+        {Other->getDbgValueRange().begin(), Other->getDbgValueRange().end()});
+  }
+
+  // Iterate in lock-step until any of the DPValue lists are exausted. If
+  // the lock-step DPValues are identical, hoist all of them to TI.
+  // This replicates the dbg.* intrinsic behaviour in
+  // hoistCommonCodeFromSuccessors.
+  while (!atEnd()) {
+    bool HoistDPVs = allIdentical();
+    for (CurrentAndEndIt &Pair : Itrs) {
+      // Increment Current iterator now as we may be about to move the DPValue.
+      DPValue &DPV = *Pair.first++;
+      if (HoistDPVs) {
+        DPV.removeFromParent();
+        TI->getParent()->insertDPValueBefore(&DPV, TI->getIterator());
+      }
+    }
+  }
+}
+
 /// Hoist any common code in the successor blocks up into the block. This
 /// function guarantees that BB dominates all successors. If EqTermsOnly is
 /// given, only perform hoisting in case both blocks only contain a terminator.
@@ -1624,18 +1681,23 @@ bool SimplifyCFGOpt::hoistCommonCodeFromSuccessors(BasicBlock *BB,
         AllInstsAreIdentical = false;
     }
 
+    SmallVector<Instruction *, 8> OtherInsts;
+    for (auto &SuccIter : OtherSuccIterRange)
+      OtherInsts.push_back(&*SuccIter);
+
     // If we are hoisting the terminator instruction, don't move one (making a
     // broken BB), instead clone it, and remove BI.
     if (HasTerminator) {
       // Even if BB, which contains only one unreachable instruction, is ignored
       // at the beginning of the loop, we can hoist the terminator instruction.
       // If any instructions remain in the block, we cannot hoist terminators.
-      if (NumSkipped || !AllInstsAreIdentical)
+      if (NumSkipped || !AllInstsAreIdentical) {
+        hoistLockstepIdenticalDPValues(TI, I1, OtherInsts);
         return Changed;
-      SmallVector<Instruction *, 8> Insts;
-      for (auto &SuccIter : OtherSuccIterRange)
-        Insts.push_back(&*SuccIter);
-      return hoistSuccIdenticalTerminatorToSwitchOrIf(TI, I1, Insts) || Changed;
+      }
+
+      return hoistSuccIdenticalTerminatorToSwitchOrIf(TI, I1, OtherInsts) ||
+             Changed;
     }
 
     if (AllInstsAreIdentical) {
@@ -1660,18 +1722,25 @@ bool SimplifyCFGOpt::hoistCommonCodeFromSuccessors(BasicBlock *BB,
         // The debug location is an integral part of a debug info intrinsic
         // and can't be separated from it or replaced.  Instead of attempting
         // to merge locations, simply hoist both copies of the intrinsic.
-        I1->moveBeforePreserving(TI);
+        hoistLockstepIdenticalDPValues(TI, I1, OtherInsts);
+        // We've just hoisted DPValues; move I1 after them (before TI) and
+        // leave any that were not hoisted behind (by calling moveBefore
+        // rather than moveBeforePreserving).
+        I1->moveBefore(TI);
         for (auto &SuccIter : OtherSuccIterRange) {
           auto *I2 = &*SuccIter++;
           assert(isa<DbgInfoIntrinsic>(I2));
-          I2->moveBeforePreserving(TI);
+          I2->moveBefore(TI);
         }
       } else {
         // For a normal instruction, we just move one to right before the
         // branch, then replace all uses of the other with the first.  Finally,
-        // we remove the now redundant second instruction.
-        I1->moveBeforePreserving(TI);
-        BB->splice(TI->getIterator(), BB1, I1->getIterator());
+        // we remove the now redundant second instruction.s
+        hoistLockstepIdenticalDPValues(TI, I1, OtherInsts);
+        // We've just hoisted DPValues; move I1 after them (before TI) and
+        // leave any that were not hoisted behind (by calling moveBefore
+        // rather than moveBeforePreserving).
+        I1->moveBefore(TI);
         for (auto &SuccIter : OtherSuccIterRange) {
           Instruction *I2 = &*SuccIter++;
           assert(I2 != I1);
@@ -1690,8 +1759,10 @@ bool SimplifyCFGOpt::hoistCommonCodeFromSuccessors(BasicBlock *BB,
       Changed = true;
       NumHoistCommonInstrs += SuccIterPairs.size();
     } else {
-      if (NumSkipped >= HoistCommonSkipLimit)
+      if (NumSkipped >= HoistCommonSkipLimit) {
+        hoistLockstepIdenticalDPValues(TI, I1, OtherInsts);
         return Changed;
+      }
       // We are about to skip over a pair of non-identical instructions. Record
       // if any have characteristics that would prevent reordering instructions
       // across them.
@@ -1752,9 +1823,13 @@ bool SimplifyCFGOpt::hoistSuccIdenticalTerminatorToSwitchOrIf(
     }
   }
 
-  // Okay, it is safe to hoist the terminator.
+  // Hoist DPValues attached to the terminator to match dbg.* intrinsic hoisting
+  // behaviour in hoistCommonCodeFromSuccessors.
+  hoistLockstepIdenticalDPValues(TI, I1, OtherSuccTIs);
+  // Clone the terminator and hoist it into the pred, without any debug info.
   Instruction *NT = I1->clone();
   NT->insertInto(TIParent, TI->getIterator());
+
   if (!NT->getType()->isVoidTy()) {
     I1->replaceAllUsesWith(NT);
     for (Instruction *OtherSuccTI : OtherSuccTIs)
diff --git a/llvm/test/Transforms/SimplifyCFG/X86/pr39187-g.ll b/llvm/test/Transforms/SimplifyCFG/X86/pr39187-g.ll
index 52dc284c7959486..8cc466ff82e5d90 100644
--- a/llvm/test/Transforms/SimplifyCFG/X86/pr39187-g.ll
+++ b/llvm/test/Transforms/SimplifyCFG/X86/pr39187-g.ll
@@ -1,4 +1,5 @@
 ; RUN: opt < %s -S -passes=simplifycfg -simplifycfg-require-and-preserve-domtree=1 -hoist-common-insts=true | FileCheck %s
+; RUN: opt --try-experimental-debuginfo-iterators < %s -S -passes=simplifycfg -simplifycfg-require-and-preserve-domtree=1 -hoist-common-insts=true | FileCheck %s
 
 ; SimplifyCFG can hoist any common code in the 'then' and 'else' blocks to
 ; the 'if' basic block.
diff --git a/llvm/test/Transforms/SimplifyCFG/hoist-dbgvalue-inlined.ll b/llvm/test/Transforms/SimplifyCFG/hoist-dbgvalue-inlined.ll
index bdf4802480abf12..e00d1daf71de58c 100644
--- a/llvm/test/Transforms/SimplifyCFG/hoist-dbgvalue-inlined.ll
+++ b/llvm/test/Transforms/SimplifyCFG/hoist-dbgvalue-inlined.ll
@@ -1,4 +1,5 @@
 ; RUN: opt -passes=simplifycfg -simplifycfg-require-and-preserve-domtree=1 -hoist-common-insts=true -S < %s | FileCheck %s
+; RUN: opt --try-experimental-debuginfo-iterators -passes=simplifycfg -simplifycfg-require-and-preserve-domtree=1 -hoist-common-insts=true -S < %s | FileCheck %s
 ; Verify that we don't crash due an invalid !dbg location on the hoisted llvm.dbg.value
 
 define i64 @caller(ptr %ptr, i64 %flag) !dbg !10 {
diff --git a/llvm/test/Transforms/SimplifyCFG/hoist-dbgvalue.ll b/llvm/test/Transforms/SimplifyCFG/hoist-dbgvalue.ll
index 1979c5096ab629e..af7da45ec089ccc 100644
--- a/llvm/test/Transforms/SimplifyCFG/hoist-dbgvalue.ll
+++ b/llvm/test/Transforms/SimplifyCFG/hoist-dbgvalue.ll
@@ -1,5 +1,6 @@
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -passes=simplifycfg -simplifycfg-require-and-preserve-domtree=1 -S < %s | FileCheck %s
+; RUN: opt -try-experimental-debuginfo-iterators -passes=simplifycfg -simplifycfg-require-and-preserve-domtree=1 -S < %s | FileCheck %s
 
 define i32 @foo(i32 %i) nounwind ssp !dbg !0 {
 ; CHECK-LABEL: @foo(

[SLTozer]

Looks like it does what it says on the tin, some minor comments inline.

[SLTozer]

Suggested change

	// we remove the now redundant second instruction.s
	// we remove the now redundant second instruction.

[SLTozer]

Entirely optional, but I feel like this would be more readable if these lambdas used arguments rather than captures - the names are descriptive enough, but the subjects are not, i.e. to grok what variables we're checking you have to scroll up to read the lambda, while atEnd(Itrs) and allIdentical(Itrs) makes it a bit clearer what's going on, and even clearer would be while (none_of(Itrs, atEnd)) {...} and allIdentical(make_first_range(Itrs)) - imo, ymmv, etc, this is purely a subjective and not a requirement to merge.

[OCHyams]

Thanks for the review! I agree with your style comments, merging with those changes.