[AArch64] Decompose constant multiplies used only by memory addresses - #194584
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: Jiang Ning (JiangNingHX) ChangesAArch64 currently avoids decomposing a constant multiply when the multiply has That heuristic is too conservative when the ADD/SUB is used only as a memory Relax the bailout for ADD/SUB users that are only used as unindexed load/store Fixes #161446. Full diff: https://github.com/llvm/llvm-project/pull/194584.diff 2 Files Affected:
diff --git a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
index 8d8727593ada7..7f7bbb6852b24 100644
--- a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
+++ b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
@@ -20345,6 +20345,39 @@ static SDValue performVectorExtCombine(SDNode *N, SelectionDAG &DAG) {
return SDValue();
}
+static bool isOnlyUsedAsMemoryAddress(SDNode *N) {
+ assert((N->getOpcode() == ISD::ADD || N->getOpcode() == ISD::SUB) &&
+ "Expected add/sub node");
+
+ if (N->use_empty())
+ return false;
+
+ for (SDNode *User : N->users()) {
+ switch (User->getOpcode()) {
+ case ISD::LOAD: {
+ auto *Load = cast<LoadSDNode>(User);
+ if (!Load->isUnindexed() || Load->getBasePtr().getNode() != N)
+ return false;
+ break;
+ }
+ case ISD::STORE: {
+ auto *Store = cast<StoreSDNode>(User);
+ if (!Store->isUnindexed() || Store->getBasePtr().getNode() != N)
+ return false;
+ break;
+ }
+ case AArch64ISD::PREFETCH:
+ if (User->getOperand(2).getNode() != N)
+ return false;
+ break;
+ default:
+ return false;
+ }
+ }
+
+ return true;
+}
+
static SDValue performMulCombine(SDNode *N, SelectionDAG &DAG,
TargetLowering::DAGCombinerInfo &DCI,
const AArch64Subtarget *Subtarget) {
@@ -20430,9 +20463,12 @@ static SDValue performMulCombine(SDNode *N, SelectionDAG &DAG,
return SDValue();
// Conservatively do not lower to shift+add+shift if the mul might be
// folded into madd or msub.
- if (N->hasOneUse() && (N->user_begin()->getOpcode() == ISD::ADD ||
- N->user_begin()->getOpcode() == ISD::SUB))
- return SDValue();
+ if (N->hasOneUse()) {
+ SDNode *User = *N->user_begin();
+ if ((User->getOpcode() == ISD::ADD || User->getOpcode() == ISD::SUB) &&
+ !isOnlyUsedAsMemoryAddress(User))
+ return SDValue();
+ }
}
// Use ShiftedConstValue instead of ConstValue to support both shift+add/sub
// and shift+add+shift.
diff --git a/llvm/test/CodeGen/AArch64/mul-const-addressing-mode.ll b/llvm/test/CodeGen/AArch64/mul-const-addressing-mode.ll
new file mode 100644
index 0000000000000..2e869327c1524
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/mul-const-addressing-mode.ll
@@ -0,0 +1,120 @@
+; NOTE: Assertions have been manually written to cover llvm.org/PR161446.
+; RUN: llc < %s -mtriple=aarch64 | FileCheck %s
+
+define i32 @ldrw_3x(ptr %p, i64 %x) {
+; CHECK-LABEL: ldrw_3x:
+; CHECK-NOT: mul
+; CHECK: add x[[IDX:[0-9]+]], x1, x1, lsl #1
+; CHECK-NEXT: ldr w0, [x0, x[[IDX]], lsl #2]
+; CHECK-NEXT: ret
+ %off = mul i64 %x, 12
+ %addr = getelementptr inbounds i8, ptr %p, i64 %off
+ %v = load i32, ptr %addr, align 4
+ ret i32 %v
+}
+
+define i32 @ldrw_5x(ptr %p, i64 %x) {
+; CHECK-LABEL: ldrw_5x:
+; CHECK-NOT: mul
+; CHECK: add x[[IDX:[0-9]+]], x1, x1, lsl #2
+; CHECK-NEXT: ldr w0, [x0, x[[IDX]], lsl #2]
+; CHECK-NEXT: ret
+ %off = mul i64 %x, 20
+ %addr = getelementptr inbounds i8, ptr %p, i64 %off
+ %v = load i32, ptr %addr, align 4
+ ret i32 %v
+}
+
+define i32 @ldrw_6x(ptr %p, i64 %x) {
+; CHECK-LABEL: ldrw_6x:
+; CHECK-NOT: mul
+; CHECK: add x[[IDX:[0-9]+]], x1, x1, lsl #1
+; CHECK-NEXT: lsl x[[OFF:[0-9]+]], x[[IDX]], #3
+; CHECK-NEXT: ldr w0, [x0, x[[OFF]]]
+; CHECK-NEXT: ret
+ %off = mul i64 %x, 24
+ %addr = getelementptr inbounds i8, ptr %p, i64 %off
+ %v = load i32, ptr %addr, align 4
+ ret i32 %v
+}
+
+define void @strw_3x(ptr %p, i64 %x, i32 %v) {
+; CHECK-LABEL: strw_3x:
+; CHECK-NOT: mul
+; CHECK: add x[[IDX:[0-9]+]], x1, x1, lsl #1
+; CHECK-NEXT: str w2, [x0, x[[IDX]], lsl #2]
+; CHECK-NEXT: ret
+ %off = mul i64 %x, 12
+ %addr = getelementptr inbounds i8, ptr %p, i64 %off
+ store i32 %v, ptr %addr, align 4
+ ret void
+}
+
+define i64 @ldrx_3x(ptr %p, i64 %x) {
+; CHECK-LABEL: ldrx_3x:
+; CHECK-NOT: mul
+; CHECK: add x[[IDX:[0-9]+]], x1, x1, lsl #1
+; CHECK-NEXT: ldr x0, [x0, x[[IDX]], lsl #3]
+; CHECK-NEXT: ret
+ %off = mul i64 %x, 24
+ %addr = getelementptr inbounds i8, ptr %p, i64 %off
+ %v = load i64, ptr %addr, align 8
+ ret i64 %v
+}
+
+define i64 @ldrx_5x(ptr %p, i64 %x) {
+; CHECK-LABEL: ldrx_5x:
+; CHECK-NOT: mul
+; CHECK: add x[[IDX:[0-9]+]], x1, x1, lsl #2
+; CHECK-NEXT: ldr x0, [x0, x[[IDX]], lsl #3]
+; CHECK-NEXT: ret
+ %off = mul i64 %x, 40
+ %addr = getelementptr inbounds i8, ptr %p, i64 %off
+ %v = load i64, ptr %addr, align 8
+ ret i64 %v
+}
+
+define i64 @ldrx_6x(ptr %p, i64 %x) {
+; CHECK-LABEL: ldrx_6x:
+; CHECK-NOT: mul
+; CHECK: add x[[IDX:[0-9]+]], x1, x1, lsl #1
+; CHECK-NEXT: lsl x[[OFF:[0-9]+]], x[[IDX]], #4
+; CHECK-NEXT: ldr x0, [x0, x[[OFF]]]
+; CHECK-NEXT: ret
+ %off = mul i64 %x, 48
+ %addr = getelementptr inbounds i8, ptr %p, i64 %off
+ %v = load i64, ptr %addr, align 8
+ ret i64 %v
+}
+
+define void @strx_3x(ptr %p, i64 %x, i64 %v) {
+; CHECK-LABEL: strx_3x:
+; CHECK-NOT: mul
+; CHECK: add x[[IDX:[0-9]+]], x1, x1, lsl #1
+; CHECK-NEXT: str x2, [x0, x[[IDX]], lsl #3]
+; CHECK-NEXT: ret
+ %off = mul i64 %x, 24
+ %addr = getelementptr inbounds i8, ptr %p, i64 %off
+ store i64 %v, ptr %addr, align 8
+ ret void
+}
+
+define i64 @arithmetic_mul_add(i64 %x, i64 %y) {
+; CHECK-LABEL: arithmetic_mul_add:
+; CHECK: mov w[[C:[0-9]+]], #24
+; CHECK-NEXT: madd x0, x0, x[[C]], x1
+; CHECK-NEXT: ret
+ %mul = mul i64 %x, 24
+ %add = add i64 %mul, %y
+ ret i64 %add
+}
+
+define i64 @arithmetic_mul_sub(i64 %x, i64 %y) {
+; CHECK-LABEL: arithmetic_mul_sub:
+; CHECK: mov w[[C:[0-9]+]], #24
+; CHECK-NEXT: msub x0, x0, x[[C]], x1
+; CHECK-NEXT: ret
+ %mul = mul i64 %x, 24
+ %sub = sub i64 %y, %mul
+ ret i64 %sub
+}
|
| @@ -0,0 +1,120 @@ | |||
| ; NOTE: Assertions have been manually written to cover llvm.org/PR161446. | |||
There was a problem hiding this comment.
Please just autogenerate; the manual checks aren't helpful here.
There was a problem hiding this comment.
Done, regenerated the checks with update_llc_test_checks.py and verified the new test passes locally. Thanks!
d3bef76 to
c33ab2c
Compare
|
Ping @efriedma-quic, I regenerated the test checks with update_llc_test_checks.py as suggested and verified the new test locally. The pre-merge checks are green now. Could you take another look when you have a chance? |
efriedma-quic
left a comment
efriedma-quic
left a comment
There was a problem hiding this comment.
Have you done any checks/measurements of what effect this has in practice?
| "Expected add/sub node"); | ||
|
|
||
| if (N->use_empty()) | ||
| return false; |
There was a problem hiding this comment.
Is the use_empty check here necessary? I don't think it really does anything; it just seems to make the code more confusing.
There was a problem hiding this comment.
Removed the use_empty() check. The helper is only called for the ADD/SUB user of the multiply, so the node cannot be use-empty in the relevant path, and the loop already naturally handles the uses.
| } | ||
| case ISD::STORE: { | ||
| auto *Store = cast<StoreSDNode>(User); | ||
| if (!Store->isUnindexed() || Store->getBasePtr().getNode() != N) |
There was a problem hiding this comment.
You probably want to check the SDUse here, which you can get from N->uses(). (Not that it's really likely to matter, but there are some edge cases involving a store where the address equals the value.)
There was a problem hiding this comment.
Updated this to iterate over SDUse via N->uses() and check the operand number directly. This avoids treating a store as address-only when the same ADD/SUB is also used as the stored value. I also added a regression test for
that store value/address edge case.
Allow the existing constant-multiply decomposition to run when the multiply's single ADD/SUB user is only used as an unindexed load/store address. This keeps the MADD/MSUB-preserving bailout for arithmetic uses, while exposing ADD/LSL forms that can be folded into AArch64 register-offset addressing modes.
I have not run a full benchmark suite. I checked the generated code for the intended address-only cases and for the arithmetic cases this used to protect. For base + x*C addressing patterns, this exposes AArch64 register-offset addressing and reduces cases like mov+madd+ldr to add+ldr [base, reg, lsl]. The arithmetic mul+add/sub cases are still kept as madd/msub. I also checked the base - x*C case: it does not fold into the load/store addressing mode, but the generated sequence becomes add + shifted-sub + load instead of mov + msub + load, so instruction count is unchanged and the |
c33ab2c to
00f6ba6
Compare
I think we need something more than just individual regression tests... this seems like it could trigger relatively frequently on scalar code, and there might be some unexpected interactions. There's some work going on in #190010 to make this easier... |
I ran a local llvm-test-suite codegen diff for AArch64 with an armv9-a O3 configuration:
Overall this looks in line with the intended effect: the broader test-suite diff does not show a large amount of churn or a code-size regression, and the multiply-related changes are mostly reductions of |
|
Thanks for the review! Could someone please help land this? |
|
@JiangNingHX Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
…llvm#194584) AArch64 currently avoids decomposing a constant multiply when the multiply has a single ADD/SUB user, preserving the opportunity to form MADD/MSUB. That heuristic is too conservative when the ADD/SUB is used only as a memory address. In that case the ADD/SUB is selected as part of load/store address mode selection, so preserving the multiply does not produce MADD/MSUB and prevents the existing constant-multiply decomposition from exposing ADD/LSL forms usable by AArch64 register-offset addressing. Relax the bailout for ADD/SUB users that are only used as unindexed load/store base addresses. Fixes llvm#161446.
3 participants
AArch64 currently avoids decomposing a constant multiply when the multiply has
a single ADD/SUB user, preserving the opportunity to form MADD/MSUB.
That heuristic is too conservative when the ADD/SUB is used only as a memory
address. In that case the ADD/SUB is selected as part of load/store address
mode selection, so preserving the multiply does not produce MADD/MSUB and
prevents the existing constant-multiply decomposition from exposing ADD/LSL
forms usable by AArch64 register-offset addressing.
Relax the bailout for ADD/SUB users that are only used as unindexed load/store
base addresses.
Fixes #161446.