Refactor recomputeLiveIns to converge on added MachineBasicBlocks #79940
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-backend-x86 @llvm/pr-subscribers-backend-arm Author: Oskar Wirga (oskarwirga) ChangesThis is a fix for the regression seen in #79498 > Currently, the way that recomputeLiveIns works is that it will recompute the livein registers for that MachineBasicBlock but it matters what order you call recomputeLiveIn which can result in incorrect register allocations down the line. Now we do not recompute the entire CFG but we do ensure that the newly added MBB do reach convergence. Full diff: https://github.com/llvm/llvm-project/pull/79940.diff 12 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/LivePhysRegs.h b/llvm/include/llvm/CodeGen/LivePhysRegs.h
index 76bb34d270a26..1d40b1cbb0eaa 100644
--- a/llvm/include/llvm/CodeGen/LivePhysRegs.h
+++ b/llvm/include/llvm/CodeGen/LivePhysRegs.h
@@ -193,11 +193,18 @@ void addLiveIns(MachineBasicBlock &MBB, const LivePhysRegs &LiveRegs);
void computeAndAddLiveIns(LivePhysRegs &LiveRegs,
MachineBasicBlock &MBB);
-/// Convenience function for recomputing live-in's for \p MBB.
-static inline void recomputeLiveIns(MachineBasicBlock &MBB) {
+/// Convenience function for recomputing live-in's for a MBB. Returns true if
+/// any changes were made.
+static inline bool recomputeLiveIns(MachineBasicBlock &MBB) {
LivePhysRegs LPR;
+ auto oldLiveIns = MBB.getLiveIns();
+
MBB.clearLiveIns();
computeAndAddLiveIns(LPR, MBB);
+ MBB.sortUniqueLiveIns();
+
+ auto newLiveIns = MBB.getLiveIns();
+ return oldLiveIns != newLiveIns;
}
} // end namespace llvm
diff --git a/llvm/include/llvm/CodeGen/MachineBasicBlock.h b/llvm/include/llvm/CodeGen/MachineBasicBlock.h
index c84fd281c6a54..dc2035fa598c4 100644
--- a/llvm/include/llvm/CodeGen/MachineBasicBlock.h
+++ b/llvm/include/llvm/CodeGen/MachineBasicBlock.h
@@ -111,6 +111,10 @@ class MachineBasicBlock
RegisterMaskPair(MCPhysReg PhysReg, LaneBitmask LaneMask)
: PhysReg(PhysReg), LaneMask(LaneMask) {}
+
+ bool operator==(const RegisterMaskPair &other) const {
+ return PhysReg == other.PhysReg && LaneMask == other.LaneMask;
+ }
};
private:
@@ -473,6 +477,8 @@ class MachineBasicBlock
/// Remove entry from the livein set and return iterator to the next.
livein_iterator removeLiveIn(livein_iterator I);
+ std::vector<RegisterMaskPair> getLiveIns() const { return LiveIns; }
+
class liveout_iterator {
public:
using iterator_category = std::input_iterator_tag;
diff --git a/llvm/lib/CodeGen/BranchFolding.cpp b/llvm/lib/CodeGen/BranchFolding.cpp
index a9f78358e57b9..ecf7bc30913f5 100644
--- a/llvm/lib/CodeGen/BranchFolding.cpp
+++ b/llvm/lib/CodeGen/BranchFolding.cpp
@@ -2048,8 +2048,10 @@ bool BranchFolder::HoistCommonCodeInSuccs(MachineBasicBlock *MBB) {
FBB->erase(FBB->begin(), FIB);
if (UpdateLiveIns) {
- recomputeLiveIns(*TBB);
- recomputeLiveIns(*FBB);
+ bool anyChange = false;
+ do {
+ anyChange = recomputeLiveIns(*TBB) || recomputeLiveIns(*FBB);
+ } while (anyChange);
}
++NumHoist;
diff --git a/llvm/lib/Target/AArch64/AArch64FrameLowering.cpp b/llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
index cffd414221c30..2d0ca6e6d0d3f 100644
--- a/llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
+++ b/llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
@@ -4339,8 +4339,10 @@ AArch64FrameLowering::inlineStackProbeLoopExactMultiple(
ExitMBB->transferSuccessorsAndUpdatePHIs(&MBB);
MBB.addSuccessor(LoopMBB);
// Update liveins.
- recomputeLiveIns(*LoopMBB);
- recomputeLiveIns(*ExitMBB);
+ bool anyChange = false;
+ do {
+ anyChange = recomputeLiveIns(*ExitMBB) || recomputeLiveIns(*LoopMBB);
+ } while (anyChange);
return ExitMBB->begin();
}
diff --git a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
index 13e9d9725cc2e..9b4bb7c88bc82 100644
--- a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
+++ b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
@@ -9587,9 +9587,13 @@ AArch64InstrInfo::probedStackAlloc(MachineBasicBlock::iterator MBBI,
// Update liveins.
if (MF.getRegInfo().reservedRegsFrozen()) {
- recomputeLiveIns(*LoopTestMBB);
- recomputeLiveIns(*LoopBodyMBB);
- recomputeLiveIns(*ExitMBB);
+ bool anyChange = false;
+ do {
+ anyChange = recomputeLiveIns(*ExitMBB) ||
+ recomputeLiveIns(*LoopBodyMBB) ||
+ recomputeLiveIns(*LoopTestMBB);
+ } while (anyChange);
+ ;
}
return ExitMBB->begin();
diff --git a/llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp b/llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
index 5c1c7046fdbff..8629551152cb6 100644
--- a/llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
+++ b/llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
@@ -1806,12 +1806,13 @@ void ARMLowOverheadLoops::Expand(LowOverheadLoop &LoLoop) {
PostOrderLoopTraversal DFS(LoLoop.ML, *MLI);
DFS.ProcessLoop();
const SmallVectorImpl<MachineBasicBlock*> &PostOrder = DFS.getOrder();
- for (auto *MBB : PostOrder) {
- recomputeLiveIns(*MBB);
- // FIXME: For some reason, the live-in print order is non-deterministic for
- // our tests and I can't out why... So just sort them.
- MBB->sortUniqueLiveIns();
- }
+ bool anyChange = false;
+ do {
+ anyChange = false;
+ for (auto *MBB : PostOrder) {
+ anyChange = recomputeLiveIns(*MBB) || anyChange;
+ }
+ } while (anyChange);
for (auto *MBB : reverse(PostOrder))
recomputeLivenessFlags(*MBB);
diff --git a/llvm/lib/Target/PowerPC/PPCExpandAtomicPseudoInsts.cpp b/llvm/lib/Target/PowerPC/PPCExpandAtomicPseudoInsts.cpp
index aee57a5075ff7..b43eee8fdd8c0 100644
--- a/llvm/lib/Target/PowerPC/PPCExpandAtomicPseudoInsts.cpp
+++ b/llvm/lib/Target/PowerPC/PPCExpandAtomicPseudoInsts.cpp
@@ -208,8 +208,10 @@ bool PPCExpandAtomicPseudo::expandAtomicRMW128(
.addMBB(LoopMBB);
CurrentMBB->addSuccessor(LoopMBB);
CurrentMBB->addSuccessor(ExitMBB);
- recomputeLiveIns(*LoopMBB);
- recomputeLiveIns(*ExitMBB);
+ bool anyChange = false;
+ do {
+ anyChange = recomputeLiveIns(*ExitMBB) || recomputeLiveIns(*LoopMBB);
+ } while (anyChange);
NMBBI = MBB.end();
MI.eraseFromParent();
return true;
@@ -286,9 +288,11 @@ bool PPCExpandAtomicPseudo::expandAtomicCmpSwap128(
CurrentMBB->addSuccessor(LoopCmpMBB);
CurrentMBB->addSuccessor(ExitMBB);
- recomputeLiveIns(*LoopCmpMBB);
- recomputeLiveIns(*CmpSuccMBB);
- recomputeLiveIns(*ExitMBB);
+ bool anyChange = false;
+ do {
+ anyChange = recomputeLiveIns(*ExitMBB) || recomputeLiveIns(*CmpSuccMBB) ||
+ recomputeLiveIns(*LoopCmpMBB);
+ } while (anyChange);
NMBBI = MBB.end();
MI.eraseFromParent();
return true;
diff --git a/llvm/lib/Target/PowerPC/PPCFrameLowering.cpp b/llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
index 245e78641ed65..6792842f8550c 100644
--- a/llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
+++ b/llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
@@ -1441,8 +1441,11 @@ void PPCFrameLowering::inlineStackProbe(MachineFunction &MF,
ProbeLoopBodyMBB->addSuccessor(ProbeLoopBodyMBB);
}
// Update liveins.
- recomputeLiveIns(*ProbeLoopBodyMBB);
- recomputeLiveIns(*ProbeExitMBB);
+ bool anyChange = false;
+ do {
+ anyChange = recomputeLiveIns(*ProbeExitMBB) ||
+ recomputeLiveIns(*ProbeLoopBodyMBB);
+ } while (anyChange);
return ProbeExitMBB;
};
// For case HasBP && MaxAlign > 1, we have to realign the SP by performing
@@ -1534,8 +1537,10 @@ void PPCFrameLowering::inlineStackProbe(MachineFunction &MF,
buildDefCFAReg(*ExitMBB, ExitMBB->begin(), SPReg);
}
// Update liveins.
- recomputeLiveIns(*LoopMBB);
- recomputeLiveIns(*ExitMBB);
+ bool anyChange = false;
+ do {
+ anyChange = recomputeLiveIns(*ExitMBB) || recomputeLiveIns(*LoopMBB);
+ } while (anyChange);
}
}
++NumPrologProbed;
diff --git a/llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp b/llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp
index db19c8881c685..80c994a32ea96 100644
--- a/llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp
+++ b/llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp
@@ -840,8 +840,10 @@ void SystemZELFFrameLowering::inlineStackProbe(
StackAllocMI->eraseFromParent();
if (DoneMBB != nullptr) {
// Compute the live-in lists for the new blocks.
- recomputeLiveIns(*DoneMBB);
- recomputeLiveIns(*LoopMBB);
+ bool anyChange = false;
+ do {
+ anyChange = recomputeLiveIns(*DoneMBB) || recomputeLiveIns(*LoopMBB);
+ } while (anyChange);
}
}
@@ -1439,8 +1441,10 @@ void SystemZXPLINKFrameLowering::inlineStackProbe(
StackAllocMI->eraseFromParent();
// Compute the live-in lists for the new blocks.
- recomputeLiveIns(*NextMBB);
- recomputeLiveIns(*StackExtMBB);
+ bool anyChange = false;
+ do {
+ anyChange = recomputeLiveIns(*StackExtMBB) || recomputeLiveIns(*NextMBB);
+ } while (anyChange);
}
bool SystemZXPLINKFrameLowering::hasFP(const MachineFunction &MF) const {
diff --git a/llvm/lib/Target/X86/X86FrameLowering.cpp b/llvm/lib/Target/X86/X86FrameLowering.cpp
index c0d358ead2787..c2f76a3b8abbe 100644
--- a/llvm/lib/Target/X86/X86FrameLowering.cpp
+++ b/llvm/lib/Target/X86/X86FrameLowering.cpp
@@ -885,8 +885,10 @@ void X86FrameLowering::emitStackProbeInlineGenericLoop(
}
// Update Live In information
- recomputeLiveIns(*testMBB);
- recomputeLiveIns(*tailMBB);
+ bool anyChange = false;
+ do {
+ anyChange = recomputeLiveIns(*tailMBB) || recomputeLiveIns(*testMBB);
+ } while (anyChange);
}
void X86FrameLowering::emitStackProbeInlineWindowsCoreCLR64(
@@ -1378,10 +1380,11 @@ void X86FrameLowering::BuildStackAlignAND(MachineBasicBlock &MBB,
footMBB->addSuccessor(&MBB);
}
- recomputeLiveIns(*headMBB);
- recomputeLiveIns(*bodyMBB);
- recomputeLiveIns(*footMBB);
- recomputeLiveIns(MBB);
+ bool anyChange = false;
+ do {
+ anyChange = recomputeLiveIns(*footMBB) || recomputeLiveIns(*bodyMBB) ||
+ recomputeLiveIns(*headMBB) || recomputeLiveIns(MBB);
+ } while (anyChange);
}
} else {
MachineInstr *MI = BuildMI(MBB, MBBI, DL, TII.get(AndOp), Reg)
diff --git a/llvm/test/CodeGen/SystemZ/branch-folder-hoist-livein.mir b/llvm/test/CodeGen/SystemZ/branch-folder-hoist-livein.mir
index 5e100b88ead30..82e3bae97ec0c 100644
--- a/llvm/test/CodeGen/SystemZ/branch-folder-hoist-livein.mir
+++ b/llvm/test/CodeGen/SystemZ/branch-folder-hoist-livein.mir
@@ -1,3 +1,4 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 4
# RUN: llc -verify-machineinstrs -O1 -mtriple=s390x-ibm-linux -o - %s -run-pass=branch-folder | FileCheck %s
--- |
target datalayout = "E-m:e-i1:8:16-i8:8:16-i64:64-f128:64-a:8:16-n32:64"
@@ -15,6 +16,30 @@
name: f1
tracksRegLiveness: true
body: |
+ ; CHECK-LABEL: name: f1
+ ; CHECK: bb.0:
+ ; CHECK-NEXT: successors: %bb.2(0x7fffffff), %bb.1(0x00000001)
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: renamable $r1d = LGRL @b :: (load (s32) from got, align 8)
+ ; CHECK-NEXT: renamable $r1l = LH killed renamable $r1d, 0, $noreg, implicit-def $r1d :: (dereferenceable load (s8) from @b)
+ ; CHECK-NEXT: renamable $r2l = LHI 0
+ ; CHECK-NEXT: renamable $r3d = LGRL @d :: (load (s32) from got, align 8)
+ ; CHECK-NEXT: renamable $r4d = LLILL 0, implicit-def $r4q
+ ; CHECK-NEXT: renamable $r4d = COPY killed renamable $r4d, implicit killed $r4q
+ ; CHECK-NEXT: CHI killed renamable $r2l, 0, implicit-def $cc
+ ; CHECK-NEXT: BRC 14, 6, %bb.2, implicit killed $cc
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: bb.1:
+ ; CHECK-NEXT: successors:
+ ; CHECK-NEXT: liveins: $r3d, $r4d, $r1l
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: STH renamable $r1l, killed renamable $r3d, 0, $noreg, implicit killed $r4d :: (store (s8) into @d)
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: bb.2:
+ ; CHECK-NEXT: liveins: $r3d, $r4d, $r1l
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: STH renamable $r1l, killed renamable $r3d, 0, $noreg, implicit killed $r4d :: (store (s8) into @d)
+ ; CHECK-NEXT: Return
bb.0:
successors: %bb.2(0x7fffffff), %bb.1(0x00000001)
liveins:
@@ -44,14 +69,3 @@ body: |
Return
...
-
-# CHECK: renamable $r4d = COPY killed renamable $r4d, implicit killed $r4q
-# CHECK-NEXT: CHI killed renamable $r2l, 0, implicit-def $cc
-# CHECK-NEXT: BRC 14, 6, %bb.2, implicit killed $cc
-# CHECK-NEXT: {{^ $}}
-# CHECK-NEXT: bb.1:
-# CHECK-NEXT: successors:
-# CHECK-NEXT: liveins: $r1l, $r3d, $r4d
-
-# CHECK: bb.2:
-# CHECK-NEXT: liveins: $r1l, $r3d, $r4d
diff --git a/llvm/test/CodeGen/Thumb2/LowOverheadLoops/spillingmove.mir b/llvm/test/CodeGen/Thumb2/LowOverheadLoops/spillingmove.mir
index 1bd1d6b99e422..15aa62d5cff6b 100644
--- a/llvm/test/CodeGen/Thumb2/LowOverheadLoops/spillingmove.mir
+++ b/llvm/test/CodeGen/Thumb2/LowOverheadLoops/spillingmove.mir
@@ -336,7 +336,7 @@ body: |
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: bb.4:
; CHECK-NEXT: successors: %bb.5(0x04000000), %bb.2(0x7c000000)
- ; CHECK-NEXT: liveins: $q0, $r0, $r1, $r2, $r3, $r6, $r12
+ ; CHECK-NEXT: liveins: $d0, $d1, $r0, $r1, $r2, $r3, $r6, $r12
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: renamable $r3, dead $cpsr = nuw nsw tADDi8 killed renamable $r3, 1, 14 /* CC::al */, $noreg
; CHECK-NEXT: renamable $r0 = tADDhirr killed renamable $r0, renamable $r1, 14 /* CC::al */, $noreg
|
|
CC: @nikic would it be possible to check compile time regressions ahead of merging? |
|
@oskarwirga There is no impact on compile-time with the new patch: https://llvm-compile-time-tracker.com/compare.php?from=cb46c6181770dabad59cc738ad1d26ad78b5f885&to=265d88f922e8b68e8a19af8e41c32e6c6491c831&stat=instructions:u |
yozhu
approved these changes
Jan 30, 2024
oskarwirga
added a commit
to oskarwirga/llvm-project
that referenced
this pull request
Jan 31, 2024
…vm#79940) This is a fix for the regression seen in llvm#79498 > Currently, the way that recomputeLiveIns works is that it will recompute the livein registers for that MachineBasicBlock but it matters what order you call recomputeLiveIn which can result in incorrect register allocations down the line. Now we do not recompute the entire CFG but we do ensure that the newly added MBB do reach convergence. (cherry picked from commit ff4636a)
redstar
added a commit
to redstar/llvm-project
that referenced
this pull request
Apr 8, 2024
llvm#79940 put calls to recomputeLiveIns into a loop, to repeatedly call the function until the computation converges. However, this repeats a lot of code. This changes moves the loop into a function to simplify the handling. Note that this changes the order in which recomputeLiveIns is called. For example, ``` bool anyChange = false; do { anyChange = recomputeLiveIns(*ExitMBB) || recomputeLiveIns(*LoopMBB); } while (anyChange); ``` only begins to recompute the live-ins for LoopMBB after the computation for ExitMBB has converged. With this change, all basic blocks have a recomputation of the live-ins for each loop iteration. This can result in less or more calls, depending on the situation.
redstar
added a commit
to redstar/llvm-project
that referenced
this pull request
Apr 15, 2024
llvm#79940 put calls to recomputeLiveIns into a loop, to repeatedly call the function until the computation converges. However, this repeats a lot of code. This changes moves the loop into a function to simplify the handling. Note that this changes the order in which recomputeLiveIns is called. For example, ``` bool anyChange = false; do { anyChange = recomputeLiveIns(*ExitMBB) || recomputeLiveIns(*LoopMBB); } while (anyChange); ``` only begins to recompute the live-ins for LoopMBB after the computation for ExitMBB has converged. With this change, all basic blocks have a recomputation of the live-ins for each loop iteration. This can result in less or more calls, depending on the situation.
redstar
added a commit
that referenced
this pull request
Apr 15, 2024
#79940 put calls to recomputeLiveIns into a loop, to repeatedly call the function until the computation converges. However, this repeats a lot of code. This changes moves the loop into a function to simplify the handling. Note that this changes the order in which recomputeLiveIns is called. For example, ``` bool anyChange = false; do { anyChange = recomputeLiveIns(*ExitMBB) || recomputeLiveIns(*LoopMBB); } while (anyChange); ``` only begins to recompute the live-ins for LoopMBB after the computation for ExitMBB has converged. With this change, all basic blocks have a recomputation of the live-ins for each loop iteration. This can result in less or more calls, depending on the situation.
aniplcc
pushed a commit
to aniplcc/llvm-project
that referenced
this pull request
Apr 15, 2024
llvm#79940 put calls to recomputeLiveIns into a loop, to repeatedly call the function until the computation converges. However, this repeats a lot of code. This changes moves the loop into a function to simplify the handling. Note that this changes the order in which recomputeLiveIns is called. For example, ``` bool anyChange = false; do { anyChange = recomputeLiveIns(*ExitMBB) || recomputeLiveIns(*LoopMBB); } while (anyChange); ``` only begins to recompute the live-ins for LoopMBB after the computation for ExitMBB has converged. With this change, all basic blocks have a recomputation of the live-ins for each loop iteration. This can result in less or more calls, depending on the situation.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a fix for the regression seen in #79498
Now we do not recompute the entire CFG but we do ensure that the newly added MBB do reach convergence.