[AArch64LoadStoreOpt] BaseReg update is searched also in CF successor #145583
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-backend-aarch64 Author: Sergey Shcherbinin (SergeyShch01) ChangesLook for reg update instruction (to merge w/ mem instruction into pre/post-increment form) not only inside a single MBB but also along a CF path going downward w/o side enters such that BaseReg is alive along it but not at its exits. Regression test is updated accordingly. Full diff: https://github.com/llvm/llvm-project/pull/145583.diff 2 Files Affected:
diff --git a/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp b/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
index f51f0d11ef9d8..f46a3e7a4333e 100644
--- a/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
+++ b/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
@@ -2529,30 +2529,63 @@ MachineBasicBlock::iterator AArch64LoadStoreOpt::findMatchingUpdateInsnForward(
return E;
}
- for (unsigned Count = 0; MBBI != E && Count < Limit;
- MBBI = next_nodbg(MBBI, E)) {
- MachineInstr &MI = *MBBI;
-
- // Don't count transient instructions towards the search limit since there
- // may be different numbers of them if e.g. debug information is present.
- if (!MI.isTransient())
- ++Count;
-
- // If we found a match, return it.
- if (isMatchingUpdateInsn(*I, MI, BaseReg, UnscaledOffset))
- return MBBI;
-
- // Update the status of what the instruction clobbered and used.
- LiveRegUnits::accumulateUsedDefed(MI, ModifiedRegUnits, UsedRegUnits, TRI);
+ MachineBasicBlock *CurMBB = I->getParent();
+ // choice of next block to visit is liveins-based
+ bool VisitSucc = CurMBB->getParent()->getRegInfo().tracksLiveness();
+
+ while (true) {
+ MachineBasicBlock::iterator CurEnd = CurMBB->end();
+
+ for (unsigned Count = 0; MBBI != CurEnd && Count < Limit;
+ MBBI = next_nodbg(MBBI, CurEnd)) {
+ MachineInstr &MI = *MBBI;
+
+ // Don't count transient instructions towards the search limit since there
+ // may be different numbers of them if e.g. debug information is present.
+ if (!MI.isTransient())
+ ++Count;
+
+ // If we found a match, return it.
+ if (isMatchingUpdateInsn(*I, MI, BaseReg, UnscaledOffset))
+ return MBBI;
+
+ // Update the status of what the instruction clobbered and used.
+ LiveRegUnits::accumulateUsedDefed(MI, ModifiedRegUnits, UsedRegUnits,
+ TRI);
+
+ // Otherwise, if the base register is used or modified, we have no match,
+ // so return early. If we are optimizing SP, do not allow instructions
+ // that may load or store in between the load and the optimized value
+ // update.
+ if (!ModifiedRegUnits.available(BaseReg) ||
+ !UsedRegUnits.available(BaseReg) ||
+ (BaseRegSP && MBBI->mayLoadOrStore()))
+ return E;
+ }
- // Otherwise, if the base register is used or modified, we have no match, so
- // return early.
- // If we are optimizing SP, do not allow instructions that may load or store
- // in between the load and the optimized value update.
- if (!ModifiedRegUnits.available(BaseReg) ||
- !UsedRegUnits.available(BaseReg) ||
- (BaseRegSP && MBBI->mayLoadOrStore()))
- return E;
+ if (VisitSucc) {
+ // Try to go downward to successors along a CF path w/o side enters
+ // such that BaseReg is alive along it but not at its exits
+ MachineBasicBlock *SuccToVisit = nullptr;
+ unsigned LiveSuccCount = 0;
+ for (MachineBasicBlock *Succ : CurMBB->successors()) {
+ if (Succ->isLiveIn(BaseReg)) {
+ if (LiveSuccCount++) {
+ return E;
+ }
+ if (Succ->pred_size() == 1) {
+ SuccToVisit = Succ;
+ }
+ }
+ }
+ if (!SuccToVisit) {
+ break;
+ }
+ CurMBB = SuccToVisit;
+ MBBI = CurMBB->begin();
+ } else {
+ break;
+ }
}
return E;
}
diff --git a/llvm/test/Transforms/LoopStrengthReduce/AArch64/pr53625.ll b/llvm/test/Transforms/LoopStrengthReduce/AArch64/pr53625.ll
index 3c1094f2ee31d..ff2527d5bb6ad 100644
--- a/llvm/test/Transforms/LoopStrengthReduce/AArch64/pr53625.ll
+++ b/llvm/test/Transforms/LoopStrengthReduce/AArch64/pr53625.ll
@@ -131,12 +131,11 @@ define i32 @negative_test_type_is_struct(i32 %c, ptr %a, ptr %b) {
; CHECK-NEXT: mov w8, w0
; CHECK-NEXT: .LBB2_2: // %for.body
; CHECK-NEXT: // =>This Inner Loop Header: Depth=1
-; CHECK-NEXT: ldr w9, [x1]
+; CHECK-NEXT: ldr w9, [x1], #4
; CHECK-NEXT: cbnz w9, .LBB2_5
; CHECK-NEXT: // %bb.3: // %for.cond
; CHECK-NEXT: // in Loop: Header=BB2_2 Depth=1
; CHECK-NEXT: subs x8, x8, #1
-; CHECK-NEXT: add x1, x1, #4
; CHECK-NEXT: b.ne .LBB2_2
; CHECK-NEXT: .LBB2_4:
; CHECK-NEXT: mov w0, wzr
|
|
@llvm/pr-subscribers-llvm-transforms Author: Sergey Shcherbinin (SergeyShch01) ChangesLook for reg update instruction (to merge w/ mem instruction into pre/post-increment form) not only inside a single MBB but also along a CF path going downward w/o side enters such that BaseReg is alive along it but not at its exits. Regression test is updated accordingly. Full diff: https://github.com/llvm/llvm-project/pull/145583.diff 2 Files Affected:
diff --git a/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp b/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
index f51f0d11ef9d8..f46a3e7a4333e 100644
--- a/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
+++ b/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
@@ -2529,30 +2529,63 @@ MachineBasicBlock::iterator AArch64LoadStoreOpt::findMatchingUpdateInsnForward(
return E;
}
- for (unsigned Count = 0; MBBI != E && Count < Limit;
- MBBI = next_nodbg(MBBI, E)) {
- MachineInstr &MI = *MBBI;
-
- // Don't count transient instructions towards the search limit since there
- // may be different numbers of them if e.g. debug information is present.
- if (!MI.isTransient())
- ++Count;
-
- // If we found a match, return it.
- if (isMatchingUpdateInsn(*I, MI, BaseReg, UnscaledOffset))
- return MBBI;
-
- // Update the status of what the instruction clobbered and used.
- LiveRegUnits::accumulateUsedDefed(MI, ModifiedRegUnits, UsedRegUnits, TRI);
+ MachineBasicBlock *CurMBB = I->getParent();
+ // choice of next block to visit is liveins-based
+ bool VisitSucc = CurMBB->getParent()->getRegInfo().tracksLiveness();
+
+ while (true) {
+ MachineBasicBlock::iterator CurEnd = CurMBB->end();
+
+ for (unsigned Count = 0; MBBI != CurEnd && Count < Limit;
+ MBBI = next_nodbg(MBBI, CurEnd)) {
+ MachineInstr &MI = *MBBI;
+
+ // Don't count transient instructions towards the search limit since there
+ // may be different numbers of them if e.g. debug information is present.
+ if (!MI.isTransient())
+ ++Count;
+
+ // If we found a match, return it.
+ if (isMatchingUpdateInsn(*I, MI, BaseReg, UnscaledOffset))
+ return MBBI;
+
+ // Update the status of what the instruction clobbered and used.
+ LiveRegUnits::accumulateUsedDefed(MI, ModifiedRegUnits, UsedRegUnits,
+ TRI);
+
+ // Otherwise, if the base register is used or modified, we have no match,
+ // so return early. If we are optimizing SP, do not allow instructions
+ // that may load or store in between the load and the optimized value
+ // update.
+ if (!ModifiedRegUnits.available(BaseReg) ||
+ !UsedRegUnits.available(BaseReg) ||
+ (BaseRegSP && MBBI->mayLoadOrStore()))
+ return E;
+ }
- // Otherwise, if the base register is used or modified, we have no match, so
- // return early.
- // If we are optimizing SP, do not allow instructions that may load or store
- // in between the load and the optimized value update.
- if (!ModifiedRegUnits.available(BaseReg) ||
- !UsedRegUnits.available(BaseReg) ||
- (BaseRegSP && MBBI->mayLoadOrStore()))
- return E;
+ if (VisitSucc) {
+ // Try to go downward to successors along a CF path w/o side enters
+ // such that BaseReg is alive along it but not at its exits
+ MachineBasicBlock *SuccToVisit = nullptr;
+ unsigned LiveSuccCount = 0;
+ for (MachineBasicBlock *Succ : CurMBB->successors()) {
+ if (Succ->isLiveIn(BaseReg)) {
+ if (LiveSuccCount++) {
+ return E;
+ }
+ if (Succ->pred_size() == 1) {
+ SuccToVisit = Succ;
+ }
+ }
+ }
+ if (!SuccToVisit) {
+ break;
+ }
+ CurMBB = SuccToVisit;
+ MBBI = CurMBB->begin();
+ } else {
+ break;
+ }
}
return E;
}
diff --git a/llvm/test/Transforms/LoopStrengthReduce/AArch64/pr53625.ll b/llvm/test/Transforms/LoopStrengthReduce/AArch64/pr53625.ll
index 3c1094f2ee31d..ff2527d5bb6ad 100644
--- a/llvm/test/Transforms/LoopStrengthReduce/AArch64/pr53625.ll
+++ b/llvm/test/Transforms/LoopStrengthReduce/AArch64/pr53625.ll
@@ -131,12 +131,11 @@ define i32 @negative_test_type_is_struct(i32 %c, ptr %a, ptr %b) {
; CHECK-NEXT: mov w8, w0
; CHECK-NEXT: .LBB2_2: // %for.body
; CHECK-NEXT: // =>This Inner Loop Header: Depth=1
-; CHECK-NEXT: ldr w9, [x1]
+; CHECK-NEXT: ldr w9, [x1], #4
; CHECK-NEXT: cbnz w9, .LBB2_5
; CHECK-NEXT: // %bb.3: // %for.cond
; CHECK-NEXT: // in Loop: Header=BB2_2 Depth=1
; CHECK-NEXT: subs x8, x8, #1
-; CHECK-NEXT: add x1, x1, #4
; CHECK-NEXT: b.ne .LBB2_2
; CHECK-NEXT: .LBB2_4:
; CHECK-NEXT: mov w0, wzr
|
|
Ping |
There was a problem hiding this comment.
@john-brawn-arm has been interested in post-inc lately too.
| unsigned LiveSuccCount = 0; | ||
| for (MachineBasicBlock *Succ : CurMBB->successors()) { | ||
| if (Succ->isLiveIn(BaseReg)) { | ||
| if (LiveSuccCount++) { |
There was a problem hiding this comment.
if (SuccToVisit)
Then you can remove LiveSuccCount?
There was a problem hiding this comment.
Unfortunately, no - we need to keep track of both LiveSuccCount and SuccToVisit, since there may be a successor with multiple predecessors that has BaseReg alive at its enter - while SuccToVisit tracks only successor with a single predecessor.
| @@ -131,12 +131,11 @@ define i32 @negative_test_type_is_struct(i32 %c, ptr %a, ptr %b) { | |||
| ; CHECK-NEXT: mov w8, w0 | |||
| ; CHECK-NEXT: .LBB2_2: // %for.body | |||
| ; CHECK-NEXT: // =>This Inner Loop Header: Depth=1 | |||
| ; CHECK-NEXT: ldr w9, [x1] | |||
| ; CHECK-NEXT: ldr w9, [x1], #4 | |||
There was a problem hiding this comment.
Is it worth adding mir tests for various edge cases?
There was a problem hiding this comment.
Ok, added llvm/test/CodeGen/AArch64/ldst_update_cfpath.mir.
During writing the test I realized that liveness of super-/sub-registers should be checked also (accidently wrote such test example :) ) - then improved the code.
| if (Succ->pred_size() == 1) { | ||
| SuccToVisit = Succ; |
There was a problem hiding this comment.
You can drop {} brackets from single statement if's.
| // choice of next block to visit is liveins-based | ||
| bool VisitSucc = CurMBB->getParent()->getRegInfo().tracksLiveness(); | ||
|
|
||
| while (true) { |
There was a problem hiding this comment.
It looks like this could potentially loop forever, though I haven't come up with an example where this happens. Maybe it would be better to make this a "for (Count = 0; Count < Limit;)", as I think it makes sense for the limit to apply to all instructions searched not just those in a single block.
There was a problem hiding this comment.
It looks like this could potentially loop forever
There shouldn't be an infinite loop as we travel downward along a path w/o side enters
as I think it makes sense for the limit to apply to all instructions searched not just those in a single block.
Ah, good point, thanks! This was my original intent, but then I somehow missed it. Updated to have a common limit for all visited blocks.
…m instruction into pre/post-increment form) not only inside a single MBB but also along a CF path going downward w/o side enters such that BaseReg is alive along it but not at its exits. Regression test is updated accordingly.
SergeyShch01
force-pushed
the
mem_post_inc
branch
from
0e485ef to
ecdf7ed
Compare
Look for reg update instruction (to merge w/ mem instruction into pre/post-increment form) not only inside a single MBB but also along a CF path going downward w/o side enters such that BaseReg is alive along it but not at its exits.
Regression test is updated accordingly.