[AArch64] Verify ldp/stp alignment stricter #83948
Merged
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
When ldp-aligned-only/stp-aligned-only is specified, modified to cancel ldp/stp transformation if MachineMemOperand is not present or the access size is unknown. In the previous implementation, the test passed when there was no MachineMemOperand. Also, if the size was unknown, an incorrect value was used or an assertion failed. (But actually, if there is no MachineMemOperand, it will be excluded from the target by isCandidateToMergeOrPair() before reaching the part.) A statistic NumFailedAlignmentCheck is added. NumPairCreated is modified so that it only counts if it is not cancelled.
|
@llvm/pr-subscribers-backend-aarch64 Author: Yuta Mukai (ytmukai) ChangesWhen ldp-aligned-only/stp-aligned-only is specified, modified to cancel ldp/stp transformation if MachineMemOperand is not present or the access size is unknown. A statistic NumFailedAlignmentCheck is added. NumPairCreated is modified so that it only counts if it is not cancelled. Full diff: https://github.com/llvm/llvm-project/pull/83948.diff 2 Files Affected:
diff --git a/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp b/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
index 926a89466255ca..abfb1f43a9c81a 100644
--- a/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
+++ b/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
@@ -62,6 +62,8 @@ STATISTIC(NumUnscaledPairCreated,
"Number of load/store from unscaled generated");
STATISTIC(NumZeroStoresPromoted, "Number of narrow zero stores promoted");
STATISTIC(NumLoadsFromStoresPromoted, "Number of loads from stores promoted");
+STATISTIC(NumFailedAlignmentCheck, "Number of load/store pair transformation "
+ "not passed the alignment check");
DEBUG_COUNTER(RegRenamingCounter, DEBUG_TYPE "-reg-renaming",
"Controls which pairs are considered for renaming");
@@ -2337,9 +2339,6 @@ bool AArch64LoadStoreOpt::tryToPairLdStInst(MachineBasicBlock::iterator &MBBI) {
MachineBasicBlock::iterator Paired =
findMatchingInsn(MBBI, Flags, LdStLimit, /* FindNarrowMerge = */ false);
if (Paired != E) {
- ++NumPairCreated;
- if (TII->hasUnscaledLdStOffset(MI))
- ++NumUnscaledPairCreated;
// Keeping the iterator straight is a pain, so we let the merge routine tell
// us what the next instruction is after it's done mucking about.
auto Prev = std::prev(MBBI);
@@ -2349,24 +2348,27 @@ bool AArch64LoadStoreOpt::tryToPairLdStInst(MachineBasicBlock::iterator &MBBI) {
MachineMemOperand *MemOp =
MI.memoperands_empty() ? nullptr : MI.memoperands().front();
- // Get the needed alignments to check them if
- // ldp-aligned-only/stp-aligned-only features are opted.
- uint64_t MemAlignment = MemOp ? MemOp->getAlign().value() : -1;
- uint64_t TypeAlignment = MemOp ? Align(MemOp->getSize()).value() : -1;
+ // If a load/store arrives and ldp/stp-aligned-only feature is opted, check
+ // that the alignment of the source pointer is at least double the alignment
+ // of the type.
+ if ((MI.mayLoad() && Subtarget->hasLdpAlignedOnly()) ||
+ (MI.mayStore() && Subtarget->hasStpAlignedOnly())) {
+ // If there is no size/align information, cancel the transformation.
+ if (!MemOp || !MemOp->getMemoryType().isValid()) {
+ NumFailedAlignmentCheck++;
+ return false;
+ }
- // If a load arrives and ldp-aligned-only feature is opted, check that the
- // alignment of the source pointer is at least double the alignment of the
- // type.
- if (MI.mayLoad() && Subtarget->hasLdpAlignedOnly() && MemOp &&
- MemAlignment < 2 * TypeAlignment)
- return false;
+ // Get the needed alignments to check them if
+ // ldp-aligned-only/stp-aligned-only features are opted.
+ uint64_t MemAlignment = MemOp->getAlign().value();
+ uint64_t TypeAlignment = Align(MemOp->getSize()).value();
- // If a store arrives and stp-aligned-only feature is opted, check that the
- // alignment of the source pointer is at least double the alignment of the
- // type.
- if (MI.mayStore() && Subtarget->hasStpAlignedOnly() && MemOp &&
- MemAlignment < 2 * TypeAlignment)
- return false;
+ if (MemAlignment < 2 * TypeAlignment) {
+ NumFailedAlignmentCheck++;
+ return false;
+ }
+ }
MBBI = mergePairedInsns(MBBI, Paired, Flags);
// Collect liveness info for instructions between Prev and the new position
@@ -2374,6 +2376,10 @@ bool AArch64LoadStoreOpt::tryToPairLdStInst(MachineBasicBlock::iterator &MBBI) {
for (auto I = std::next(Prev); I != MBBI; I++)
updateDefinedRegisters(*I, DefinedInBB, TRI);
+ ++NumPairCreated;
+ if (TII->hasUnscaledLdStOffset(MI))
+ ++NumUnscaledPairCreated;
+
return true;
}
return false;
diff --git a/llvm/test/CodeGen/AArch64/ldp-stp-unknown-size.mir b/llvm/test/CodeGen/AArch64/ldp-stp-unknown-size.mir
new file mode 100644
index 00000000000000..3234a7dc11f05d
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/ldp-stp-unknown-size.mir
@@ -0,0 +1,56 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 4
+# RUN: llc -O2 -mtriple=aarch64 -mcpu=ampere1 -simplify-mir -o - %s -run-pass=aarch64-ldst-opt | FileCheck %s --check-prefixes=CHECK
+# RUN: llc -O2 -mtriple=aarch64 -simplify-mir -o - %s -run-pass=aarch64-ldst-opt | FileCheck %s --check-prefixes=CHECK-DEFAULT
+
+--- |
+ define i32 @ldp_no_size_info(ptr %0) #0 {
+ %2 = ptrtoint ptr %0 to i64
+ %3 = and i64 %2, -64
+ %4 = inttoptr i64 %3 to ptr
+ %5 = load i32, ptr %4, align 4
+ %6 = getelementptr inbounds i32, ptr %4, i64 1
+ %7 = load i32, ptr %6, align 4
+ %8 = add nsw i32 %7, %5
+ ret i32 %8
+ }
+
+...
+---
+name: ldp_no_size_info
+alignment: 64
+tracksRegLiveness: true
+tracksDebugUserValues: true
+liveins:
+ - { reg: '$x0' }
+frameInfo:
+ maxAlignment: 1
+ maxCallFrameSize: 0
+machineFunctionInfo:
+ hasRedZone: false
+body: |
+ bb.0 (%ir-block.1):
+ liveins: $x0
+
+ ; CHECK-LABEL: name: ldp_no_size_info
+ ; CHECK: liveins: $x0
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: renamable $x8 = ANDXri killed renamable $x0, 7865
+ ; CHECK-NEXT: renamable $w9 = LDRWui renamable $x8, 0 :: (load unknown-size from %ir.4, align 1)
+ ; CHECK-NEXT: renamable $w8 = LDRWui killed renamable $x8, 1 :: (load unknown-size from %ir.6, align 1)
+ ; CHECK-NEXT: $w0 = ADDWrs killed renamable $w8, killed renamable $w9, 0
+ ; CHECK-NEXT: RET undef $lr, implicit $w0
+ ;
+ ; CHECK-DEFAULT-LABEL: name: ldp_no_size_info
+ ; CHECK-DEFAULT: liveins: $x0
+ ; CHECK-DEFAULT-NEXT: {{ $}}
+ ; CHECK-DEFAULT-NEXT: renamable $x8 = ANDXri killed renamable $x0, 7865
+ ; CHECK-DEFAULT-NEXT: renamable $w9, renamable $w8 = LDPWi renamable $x8, 0 :: (load unknown-size from %ir.4, align 1), (load unknown-size from %ir.6, align 1)
+ ; CHECK-DEFAULT-NEXT: $w0 = ADDWrs killed renamable $w8, killed renamable $w9, 0
+ ; CHECK-DEFAULT-NEXT: RET undef $lr, implicit $w0
+ renamable $x8 = ANDXri killed renamable $x0, 7865
+ renamable $w9 = LDRWui renamable $x8, 0 :: (load unknown-size from %ir.4)
+ renamable $w8 = LDRWui killed renamable $x8, 1 :: (load unknown-size from %ir.6)
+ $w0 = ADDWrs killed renamable $w8, killed renamable $w9, 0
+ RET undef $lr, implicit $w0
+
+...
|
|
I initially intended to solve the assertion failures when the size of MachineMemOperand was unknown (#80841). @manosanaggh Could you review this patch? |
davemgreen
approved these changes
Mar 5, 2024
|
Thank you for your quick review! |
|
This broke the ASan buildbot: https://lab.llvm.org/buildbot/#/builders/168/builds/19054/steps/10/logs/stdio |
fmayer
added a commit
that referenced
this pull request
Mar 5, 2024
Reverts #83948 This broke the ASan buildbot: https://lab.llvm.org/buildbot/#/builders/168/builds/19054/steps/10/logs/stdio
|
@fmayer Thank you for reverting it. |
ytmukai
commented
Mar 6, 2024
|
|
||
| MBBI = mergePairedInsns(MBBI, Paired, Flags); | ||
| // Collect liveness info for instructions between Prev and the new position | ||
| // MBBI. | ||
| for (auto I = std::next(Prev); I != MBBI; I++) | ||
| updateDefinedRegisters(*I, DefinedInBB, TRI); | ||
|
|
||
| ++NumPairCreated; | ||
| if (TII->hasUnscaledLdStOffset(MI)) |
There was a problem hiding this comment.
This line must precede the update. I'll modify it and resubmit a patch.
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.
When ldp-aligned-only/stp-aligned-only is specified, modified to cancel ldp/stp transformation if MachineMemOperand is not present or the access size is unknown.
In the previous implementation, the test passed when there was no MachineMemOperand. Also, if the size was unknown, an incorrect value was used or an assertion failed. (But actually, if there is no MachineMemOperand, it will be excluded from the target by isCandidateToMergeOrPair() before reaching the part.)
A statistic NumFailedAlignmentCheck is added. NumPairCreated is modified so that it only counts if it is not cancelled.