[llvm] [RemoveDIs] Handle DPValues in hoistCommonCodeFromSuccessors (PR #79476)
Orlando Cazalet-Hyams via llvm-commits
llvm-commits at lists.llvm.org
Mon Feb 5 02:54:45 PST 2024
https://github.com/OCHyams updated https://github.com/llvm/llvm-project/pull/79476
>From 58dab7d7df31cfb19e976e8fd89abb42d1d93cb3 Mon Sep 17 00:00:00 2001
From: OCHyams <orlando.hyams at sony.com>
Date: Thu, 25 Jan 2024 10:34:26 +0000
Subject: [PATCH 1/4] [RemoveDIs] Handle DPValues in
hoistCommonCodeFromSuccessors
This involves hoisting DPValues that are identical in lock-step attached
to each instruction being considered for hoisting (i.e., we also need
to do it for the final instructions which are not hoisted, because
dbg.values after the last "real" instruction would be hoisted).
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
---
llvm/lib/Transforms/Utils/SimplifyCFG.cpp | 99 ++++++++++++++++---
.../Transforms/SimplifyCFG/X86/pr39187-g.ll | 1 +
.../SimplifyCFG/hoist-dbgvalue-inlined.ll | 1 +
.../Transforms/SimplifyCFG/hoist-dbgvalue.ll | 1 +
4 files changed, 90 insertions(+), 12 deletions(-)
diff --git a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
index 254795ec24453..6f2ce7b1f7645 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 52dc284c79594..8cc466ff82e5d 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 bdf4802480abf..e00d1daf71de5 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 1979c5096ab62..af7da45ec089c 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(
>From 228c82936c26795f5b80881c65b83a5c74c2b508 Mon Sep 17 00:00:00 2001
From: OCHyams <orlando.hyams at sony.com>
Date: Thu, 25 Jan 2024 17:45:58 +0000
Subject: [PATCH 2/4] undo whitespace change
---
llvm/lib/Transforms/Utils/SimplifyCFG.cpp | 1 -
1 file changed, 1 deletion(-)
diff --git a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
index 6f2ce7b1f7645..d444a4c55b8b5 100644
--- a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
+++ b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
@@ -1829,7 +1829,6 @@ bool SimplifyCFGOpt::hoistSuccIdenticalTerminatorToSwitchOrIf(
// 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)
>From ecbd30abcb29f35b3733fb387660f70a019bb89a Mon Sep 17 00:00:00 2001
From: OCHyams <orlando.hyams at sony.com>
Date: Mon, 5 Feb 2024 10:20:05 +0000
Subject: [PATCH 3/4] address review feedback
---
llvm/lib/Transforms/Utils/SimplifyCFG.cpp | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
index d444a4c55b8b5..bb8417d9b509f 100644
--- a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
+++ b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
@@ -1543,13 +1543,12 @@ hoistLockstepIdenticalDPValues(Instruction *TI, Instruction *I1,
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 this Current == End.
+ auto atEnd = [](const CurrentAndEndIt &Pair) {
+ return Pair.first == Pair.second;
};
// Return true if all Current are identical.
- auto allIdentical = [&]() {
+ auto allIdentical = [](const SmallVector<CurrentAndEndIt> &Itrs) {
return all_of(make_first_range(ArrayRef(Itrs).drop_front()),
[&](DPValue::self_iterator I) {
return Itrs[0].first->isIdenticalToWhenDefined(*I);
@@ -1570,8 +1569,8 @@ hoistLockstepIdenticalDPValues(Instruction *TI, Instruction *I1,
// 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();
+ while (none_of(Itrs, atEnd)) {
+ bool HoistDPVs = allIdentical(Itrs);
for (CurrentAndEndIt &Pair : Itrs) {
// Increment Current iterator now as we may be about to move the DPValue.
DPValue &DPV = *Pair.first++;
@@ -1735,7 +1734,7 @@ bool SimplifyCFGOpt::hoistCommonCodeFromSuccessors(BasicBlock *BB,
} 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.s
+ // we remove the now redundant second instruction.
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
>From ae2f87922cdf9cb23003b2fed9a0a827118973ac Mon Sep 17 00:00:00 2001
From: OCHyams <orlando.hyams at sony.com>
Date: Mon, 5 Feb 2024 10:44:36 +0000
Subject: [PATCH 4/4] remove now unused var
---
llvm/lib/Transforms/Utils/SimplifyCFG.cpp | 1 -
1 file changed, 1 deletion(-)
diff --git a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
index bb8417d9b509f..f5b398cae04ed 100644
--- a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
+++ b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
@@ -1654,7 +1654,6 @@ bool SimplifyCFGOpt::hoistCommonCodeFromSuccessors(BasicBlock *BB,
auto OtherSuccIterRange = make_first_range(OtherSuccIterPairRange);
Instruction *I1 = &*BB1ItrPair.first;
- auto *BB1 = I1->getParent();
// Skip debug info if it is not identical.
bool AllDbgInstsAreIdentical = all_of(OtherSuccIterRange, [I1](auto &Iter) {
More information about the llvm-commits
mailing list