PeepholeOpt: Fix looking for def of current copy to coalesce #125533
Merged
+2,720
−2,716
Conversation
This file contains 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
This stack of pull requests is managed by Graphite. Learn more about stacking. |
arsenm
requested review from
jayfoad,
kparzysz,
qcolombet,
RKSimon,
sdesmalen-arm and
wangpc-pp
|
@llvm/pr-subscribers-backend-arm @llvm/pr-subscribers-llvm-regalloc Author: Matt Arsenault (arsenm) ChangesThis fixes the handling of subregister extract copies. This The copy coalescing processing here is overly abstracted This does not fix the confused handling in the uncoalescable There are some improvements and some regressions. The The worst regression is an absurd SPARC testcase using a <251 x fp128>, We need improved subregister handling locally in PeepholeOptimizer, Patch is 475.60 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/125533.diff 105 Files Affected:
diff --git a/llvm/lib/CodeGen/PeepholeOptimizer.cpp b/llvm/lib/CodeGen/PeepholeOptimizer.cpp
index e0053fb243369c..6739199a802231 100644
--- a/llvm/lib/CodeGen/PeepholeOptimizer.cpp
+++ b/llvm/lib/CodeGen/PeepholeOptimizer.cpp
@@ -465,7 +465,8 @@ class PeepholeOptimizer : private MachineFunction::Delegate {
bool optimizeUncoalescableCopy(MachineInstr &MI,
SmallPtrSetImpl<MachineInstr *> &LocalMIs);
bool optimizeRecurrence(MachineInstr &PHI);
- bool findNextSource(RegSubRegPair RegSubReg, RewriteMapTy &RewriteMap);
+ bool findNextSource(const TargetRegisterClass *DefRC, unsigned DefSubReg,
+ RegSubRegPair RegSubReg, RewriteMapTy &RewriteMap);
bool isMoveImmediate(MachineInstr &MI, SmallSet<Register, 4> &ImmDefRegs,
DenseMap<Register, MachineInstr *> &ImmDefMIs);
bool foldImmediate(MachineInstr &MI, SmallSet<Register, 4> &ImmDefRegs,
@@ -1002,17 +1003,15 @@ bool PeepholeOptimizer::optimizeCondBranch(MachineInstr &MI) {
/// share the same register file as \p Reg and \p SubReg. The client should
/// then be capable to rewrite all intermediate PHIs to get the next source.
/// \return False if no alternative sources are available. True otherwise.
-bool PeepholeOptimizer::findNextSource(RegSubRegPair RegSubReg,
+bool PeepholeOptimizer::findNextSource(const TargetRegisterClass *DefRC,
+ unsigned DefSubReg,
+ RegSubRegPair RegSubReg,
RewriteMapTy &RewriteMap) {
// Do not try to find a new source for a physical register.
// So far we do not have any motivating example for doing that.
// Thus, instead of maintaining untested code, we will revisit that if
// that changes at some point.
Register Reg = RegSubReg.Reg;
- if (Reg.isPhysical())
- return false;
- const TargetRegisterClass *DefRC = MRI->getRegClass(Reg);
-
SmallVector<RegSubRegPair, 4> SrcToLook;
RegSubRegPair CurSrcPair = RegSubReg;
SrcToLook.push_back(CurSrcPair);
@@ -1076,7 +1075,7 @@ bool PeepholeOptimizer::findNextSource(RegSubRegPair RegSubReg,
// Keep following the chain if the value isn't any better yet.
const TargetRegisterClass *SrcRC = MRI->getRegClass(CurSrcPair.Reg);
- if (!TRI->shouldRewriteCopySrc(DefRC, RegSubReg.SubReg, SrcRC,
+ if (!TRI->shouldRewriteCopySrc(DefRC, DefSubReg, SrcRC,
CurSrcPair.SubReg))
continue;
@@ -1184,21 +1183,33 @@ bool PeepholeOptimizer::optimizeCoalescableCopyImpl(Rewriter &&CpyRewriter) {
bool Changed = false;
// Get the right rewriter for the current copy.
// Rewrite each rewritable source.
- RegSubRegPair Src;
+ RegSubRegPair Dst;
RegSubRegPair TrackPair;
- while (CpyRewriter.getNextRewritableSource(Src, TrackPair)) {
+ while (CpyRewriter.getNextRewritableSource(TrackPair, Dst)) {
+ if (Dst.Reg.isPhysical()) {
+ // Do not try to find a new source for a physical register.
+ // So far we do not have any motivating example for doing that.
+ // Thus, instead of maintaining untested code, we will revisit that if
+ // that changes at some point.
+ continue;
+ }
+
+ const TargetRegisterClass *DefRC = MRI->getRegClass(Dst.Reg);
+
// Keep track of PHI nodes and its incoming edges when looking for sources.
RewriteMapTy RewriteMap;
// Try to find a more suitable source. If we failed to do so, or get the
// actual source, move to the next source.
- if (!findNextSource(TrackPair, RewriteMap))
+ if (!findNextSource(DefRC, Dst.SubReg, TrackPair, RewriteMap))
continue;
// Get the new source to rewrite. TODO: Only enable handling of multiple
// sources (PHIs) once we have a motivating example and testcases for it.
RegSubRegPair NewSrc = getNewSource(MRI, TII, TrackPair, RewriteMap,
/*HandleMultipleSources=*/false);
- if (Src.Reg == NewSrc.Reg || NewSrc.Reg == 0)
+ assert(TrackPair.Reg != NewSrc.Reg &&
+ "should not rewrite source to original value");
+ if (!NewSrc.Reg)
continue;
// Rewrite source.
@@ -1325,9 +1336,14 @@ bool PeepholeOptimizer::optimizeUncoalescableCopy(
if (Def.Reg.isPhysical())
return false;
+ // FIXME: Uncoalescable copies are treated differently by
+ // UncoalescableRewriter, and this probably should not share
+ // API. getNextRewritableSource really finds rewritable defs.
+ const TargetRegisterClass *DefRC = MRI->getRegClass(Def.Reg);
+
// If we do not know how to rewrite this definition, there is no point
// in trying to kill this instruction.
- if (!findNextSource(Def, RewriteMap))
+ if (!findNextSource(DefRC, Def.SubReg, Def, RewriteMap))
return false;
RewritePairs.push_back(Def);
diff --git a/llvm/test/CodeGen/AArch64/Atomics/aarch64-atomicrmw-lse2.ll b/llvm/test/CodeGen/AArch64/Atomics/aarch64-atomicrmw-lse2.ll
index d93ef6f8b2869b..94d46148f37e3a 100644
--- a/llvm/test/CodeGen/AArch64/Atomics/aarch64-atomicrmw-lse2.ll
+++ b/llvm/test/CodeGen/AArch64/Atomics/aarch64-atomicrmw-lse2.ll
@@ -12,8 +12,8 @@ define dso_local i8 @atomicrmw_xchg_i8_aligned_monotonic(ptr %ptr, i8 %value) {
; -O0: subs w8, w8, w10, uxtb
;
; -O1-LABEL: atomicrmw_xchg_i8_aligned_monotonic:
-; -O1: ldxrb w8, [x0]
-; -O1: stxrb w9, w1, [x0]
+; -O1: ldxrb w0, [x8]
+; -O1: stxrb w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i8 %value monotonic, align 1
ret i8 %r
}
@@ -27,8 +27,8 @@ define dso_local i8 @atomicrmw_xchg_i8_aligned_acquire(ptr %ptr, i8 %value) {
; -O0: subs w8, w8, w10, uxtb
;
; -O1-LABEL: atomicrmw_xchg_i8_aligned_acquire:
-; -O1: ldaxrb w8, [x0]
-; -O1: stxrb w9, w1, [x0]
+; -O1: ldaxrb w0, [x8]
+; -O1: stxrb w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i8 %value acquire, align 1
ret i8 %r
}
@@ -42,8 +42,8 @@ define dso_local i8 @atomicrmw_xchg_i8_aligned_release(ptr %ptr, i8 %value) {
; -O0: subs w8, w8, w10, uxtb
;
; -O1-LABEL: atomicrmw_xchg_i8_aligned_release:
-; -O1: ldxrb w8, [x0]
-; -O1: stlxrb w9, w1, [x0]
+; -O1: ldxrb w0, [x8]
+; -O1: stlxrb w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i8 %value release, align 1
ret i8 %r
}
@@ -57,8 +57,8 @@ define dso_local i8 @atomicrmw_xchg_i8_aligned_acq_rel(ptr %ptr, i8 %value) {
; -O0: subs w8, w8, w10, uxtb
;
; -O1-LABEL: atomicrmw_xchg_i8_aligned_acq_rel:
-; -O1: ldaxrb w8, [x0]
-; -O1: stlxrb w9, w1, [x0]
+; -O1: ldaxrb w0, [x8]
+; -O1: stlxrb w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i8 %value acq_rel, align 1
ret i8 %r
}
@@ -72,8 +72,8 @@ define dso_local i8 @atomicrmw_xchg_i8_aligned_seq_cst(ptr %ptr, i8 %value) {
; -O0: subs w8, w8, w10, uxtb
;
; -O1-LABEL: atomicrmw_xchg_i8_aligned_seq_cst:
-; -O1: ldaxrb w8, [x0]
-; -O1: stlxrb w9, w1, [x0]
+; -O1: ldaxrb w0, [x8]
+; -O1: stlxrb w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i8 %value seq_cst, align 1
ret i8 %r
}
@@ -86,8 +86,8 @@ define dso_local i16 @atomicrmw_xchg_i16_aligned_monotonic(ptr %ptr, i16 %value)
; -O0: subs w8, w8, w9, uxth
;
; -O1-LABEL: atomicrmw_xchg_i16_aligned_monotonic:
-; -O1: ldxrh w8, [x0]
-; -O1: stxrh w9, w1, [x0]
+; -O1: ldxrh w0, [x8]
+; -O1: stxrh w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i16 %value monotonic, align 2
ret i16 %r
}
@@ -100,8 +100,8 @@ define dso_local i16 @atomicrmw_xchg_i16_aligned_acquire(ptr %ptr, i16 %value) {
; -O0: subs w8, w8, w9, uxth
;
; -O1-LABEL: atomicrmw_xchg_i16_aligned_acquire:
-; -O1: ldaxrh w8, [x0]
-; -O1: stxrh w9, w1, [x0]
+; -O1: ldaxrh w0, [x8]
+; -O1: stxrh w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i16 %value acquire, align 2
ret i16 %r
}
@@ -114,8 +114,8 @@ define dso_local i16 @atomicrmw_xchg_i16_aligned_release(ptr %ptr, i16 %value) {
; -O0: subs w8, w8, w9, uxth
;
; -O1-LABEL: atomicrmw_xchg_i16_aligned_release:
-; -O1: ldxrh w8, [x0]
-; -O1: stlxrh w9, w1, [x0]
+; -O1: ldxrh w0, [x8]
+; -O1: stlxrh w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i16 %value release, align 2
ret i16 %r
}
@@ -128,8 +128,8 @@ define dso_local i16 @atomicrmw_xchg_i16_aligned_acq_rel(ptr %ptr, i16 %value) {
; -O0: subs w8, w8, w9, uxth
;
; -O1-LABEL: atomicrmw_xchg_i16_aligned_acq_rel:
-; -O1: ldaxrh w8, [x0]
-; -O1: stlxrh w9, w1, [x0]
+; -O1: ldaxrh w0, [x8]
+; -O1: stlxrh w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i16 %value acq_rel, align 2
ret i16 %r
}
@@ -142,8 +142,8 @@ define dso_local i16 @atomicrmw_xchg_i16_aligned_seq_cst(ptr %ptr, i16 %value) {
; -O0: subs w8, w8, w9, uxth
;
; -O1-LABEL: atomicrmw_xchg_i16_aligned_seq_cst:
-; -O1: ldaxrh w8, [x0]
-; -O1: stlxrh w9, w1, [x0]
+; -O1: ldaxrh w0, [x8]
+; -O1: stlxrh w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i16 %value seq_cst, align 2
ret i16 %r
}
@@ -392,8 +392,8 @@ define dso_local i8 @atomicrmw_xchg_i8_unaligned_monotonic(ptr %ptr, i8 %value)
; -O0: subs w8, w8, w10, uxtb
;
; -O1-LABEL: atomicrmw_xchg_i8_unaligned_monotonic:
-; -O1: ldxrb w8, [x0]
-; -O1: stxrb w9, w1, [x0]
+; -O1: ldxrb w0, [x8]
+; -O1: stxrb w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i8 %value monotonic, align 1
ret i8 %r
}
@@ -407,8 +407,8 @@ define dso_local i8 @atomicrmw_xchg_i8_unaligned_acquire(ptr %ptr, i8 %value) {
; -O0: subs w8, w8, w10, uxtb
;
; -O1-LABEL: atomicrmw_xchg_i8_unaligned_acquire:
-; -O1: ldaxrb w8, [x0]
-; -O1: stxrb w9, w1, [x0]
+; -O1: ldaxrb w0, [x8]
+; -O1: stxrb w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i8 %value acquire, align 1
ret i8 %r
}
@@ -422,8 +422,8 @@ define dso_local i8 @atomicrmw_xchg_i8_unaligned_release(ptr %ptr, i8 %value) {
; -O0: subs w8, w8, w10, uxtb
;
; -O1-LABEL: atomicrmw_xchg_i8_unaligned_release:
-; -O1: ldxrb w8, [x0]
-; -O1: stlxrb w9, w1, [x0]
+; -O1: ldxrb w0, [x8]
+; -O1: stlxrb w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i8 %value release, align 1
ret i8 %r
}
@@ -437,8 +437,8 @@ define dso_local i8 @atomicrmw_xchg_i8_unaligned_acq_rel(ptr %ptr, i8 %value) {
; -O0: subs w8, w8, w10, uxtb
;
; -O1-LABEL: atomicrmw_xchg_i8_unaligned_acq_rel:
-; -O1: ldaxrb w8, [x0]
-; -O1: stlxrb w9, w1, [x0]
+; -O1: ldaxrb w0, [x8]
+; -O1: stlxrb w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i8 %value acq_rel, align 1
ret i8 %r
}
@@ -452,8 +452,8 @@ define dso_local i8 @atomicrmw_xchg_i8_unaligned_seq_cst(ptr %ptr, i8 %value) {
; -O0: subs w8, w8, w10, uxtb
;
; -O1-LABEL: atomicrmw_xchg_i8_unaligned_seq_cst:
-; -O1: ldaxrb w8, [x0]
-; -O1: stlxrb w9, w1, [x0]
+; -O1: ldaxrb w0, [x8]
+; -O1: stlxrb w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i8 %value seq_cst, align 1
ret i8 %r
}
diff --git a/llvm/test/CodeGen/AArch64/Atomics/aarch64-atomicrmw-rcpc.ll b/llvm/test/CodeGen/AArch64/Atomics/aarch64-atomicrmw-rcpc.ll
index 912d87dcd2b9b6..57cfeb78b69802 100644
--- a/llvm/test/CodeGen/AArch64/Atomics/aarch64-atomicrmw-rcpc.ll
+++ b/llvm/test/CodeGen/AArch64/Atomics/aarch64-atomicrmw-rcpc.ll
@@ -12,8 +12,8 @@ define dso_local i8 @atomicrmw_xchg_i8_aligned_monotonic(ptr %ptr, i8 %value) {
; -O0: subs w8, w8, w10, uxtb
;
; -O1-LABEL: atomicrmw_xchg_i8_aligned_monotonic:
-; -O1: ldxrb w8, [x0]
-; -O1: stxrb w9, w1, [x0]
+; -O1: ldxrb w0, [x8]
+; -O1: stxrb w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i8 %value monotonic, align 1
ret i8 %r
}
@@ -27,8 +27,8 @@ define dso_local i8 @atomicrmw_xchg_i8_aligned_acquire(ptr %ptr, i8 %value) {
; -O0: subs w8, w8, w10, uxtb
;
; -O1-LABEL: atomicrmw_xchg_i8_aligned_acquire:
-; -O1: ldaxrb w8, [x0]
-; -O1: stxrb w9, w1, [x0]
+; -O1: ldaxrb w0, [x8]
+; -O1: stxrb w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i8 %value acquire, align 1
ret i8 %r
}
@@ -42,8 +42,8 @@ define dso_local i8 @atomicrmw_xchg_i8_aligned_release(ptr %ptr, i8 %value) {
; -O0: subs w8, w8, w10, uxtb
;
; -O1-LABEL: atomicrmw_xchg_i8_aligned_release:
-; -O1: ldxrb w8, [x0]
-; -O1: stlxrb w9, w1, [x0]
+; -O1: ldxrb w0, [x8]
+; -O1: stlxrb w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i8 %value release, align 1
ret i8 %r
}
@@ -57,8 +57,8 @@ define dso_local i8 @atomicrmw_xchg_i8_aligned_acq_rel(ptr %ptr, i8 %value) {
; -O0: subs w8, w8, w10, uxtb
;
; -O1-LABEL: atomicrmw_xchg_i8_aligned_acq_rel:
-; -O1: ldaxrb w8, [x0]
-; -O1: stlxrb w9, w1, [x0]
+; -O1: ldaxrb w0, [x8]
+; -O1: stlxrb w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i8 %value acq_rel, align 1
ret i8 %r
}
@@ -72,8 +72,8 @@ define dso_local i8 @atomicrmw_xchg_i8_aligned_seq_cst(ptr %ptr, i8 %value) {
; -O0: subs w8, w8, w10, uxtb
;
; -O1-LABEL: atomicrmw_xchg_i8_aligned_seq_cst:
-; -O1: ldaxrb w8, [x0]
-; -O1: stlxrb w9, w1, [x0]
+; -O1: ldaxrb w0, [x8]
+; -O1: stlxrb w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i8 %value seq_cst, align 1
ret i8 %r
}
@@ -86,8 +86,8 @@ define dso_local i16 @atomicrmw_xchg_i16_aligned_monotonic(ptr %ptr, i16 %value)
; -O0: subs w8, w8, w9, uxth
;
; -O1-LABEL: atomicrmw_xchg_i16_aligned_monotonic:
-; -O1: ldxrh w8, [x0]
-; -O1: stxrh w9, w1, [x0]
+; -O1: ldxrh w0, [x8]
+; -O1: stxrh w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i16 %value monotonic, align 2
ret i16 %r
}
@@ -100,8 +100,8 @@ define dso_local i16 @atomicrmw_xchg_i16_aligned_acquire(ptr %ptr, i16 %value) {
; -O0: subs w8, w8, w9, uxth
;
; -O1-LABEL: atomicrmw_xchg_i16_aligned_acquire:
-; -O1: ldaxrh w8, [x0]
-; -O1: stxrh w9, w1, [x0]
+; -O1: ldaxrh w0, [x8]
+; -O1: stxrh w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i16 %value acquire, align 2
ret i16 %r
}
@@ -114,8 +114,8 @@ define dso_local i16 @atomicrmw_xchg_i16_aligned_release(ptr %ptr, i16 %value) {
; -O0: subs w8, w8, w9, uxth
;
; -O1-LABEL: atomicrmw_xchg_i16_aligned_release:
-; -O1: ldxrh w8, [x0]
-; -O1: stlxrh w9, w1, [x0]
+; -O1: ldxrh w0, [x8]
+; -O1: stlxrh w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i16 %value release, align 2
ret i16 %r
}
@@ -128,8 +128,8 @@ define dso_local i16 @atomicrmw_xchg_i16_aligned_acq_rel(ptr %ptr, i16 %value) {
; -O0: subs w8, w8, w9, uxth
;
; -O1-LABEL: atomicrmw_xchg_i16_aligned_acq_rel:
-; -O1: ldaxrh w8, [x0]
-; -O1: stlxrh w9, w1, [x0]
+; -O1: ldaxrh w0, [x8]
+; -O1: stlxrh w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i16 %value acq_rel, align 2
ret i16 %r
}
@@ -142,8 +142,8 @@ define dso_local i16 @atomicrmw_xchg_i16_aligned_seq_cst(ptr %ptr, i16 %value) {
; -O0: subs w8, w8, w9, uxth
;
; -O1-LABEL: atomicrmw_xchg_i16_aligned_seq_cst:
-; -O1: ldaxrh w8, [x0]
-; -O1: stlxrh w9, w1, [x0]
+; -O1: ldaxrh w0, [x8]
+; -O1: stlxrh w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i16 %value seq_cst, align 2
ret i16 %r
}
@@ -392,8 +392,8 @@ define dso_local i8 @atomicrmw_xchg_i8_unaligned_monotonic(ptr %ptr, i8 %value)
; -O0: subs w8, w8, w10, uxtb
;
; -O1-LABEL: atomicrmw_xchg_i8_unaligned_monotonic:
-; -O1: ldxrb w8, [x0]
-; -O1: stxrb w9, w1, [x0]
+; -O1: ldxrb w0, [x8]
+; -O1: stxrb w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i8 %value monotonic, align 1
ret i8 %r
}
@@ -407,8 +407,8 @@ define dso_local i8 @atomicrmw_xchg_i8_unaligned_acquire(ptr %ptr, i8 %value) {
; -O0: subs w8, w8, w10, uxtb
;
; -O1-LABEL: atomicrmw_xchg_i8_unaligned_acquire:
-; -O1: ldaxrb w8, [x0]
-; -O1: stxrb w9, w1, [x0]
+; -O1: ldaxrb w0, [x8]
+; -O1: stxrb w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i8 %value acquire, align 1
ret i8 %r
}
@@ -422,8 +422,8 @@ define dso_local i8 @atomicrmw_xchg_i8_unaligned_release(ptr %ptr, i8 %value) {
; -O0: subs w8, w8, w10, uxtb
;
; -O1-LABEL: atomicrmw_xchg_i8_unaligned_release:
-; -O1: ldxrb w8, [x0]
-; -O1: stlxrb w9, w1, [x0]
+; -O1: ldxrb w0, [x8]
+; -O1: stlxrb w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i8 %value release, align 1
ret i8 %r
}
@@ -437,8 +437,8 @@ define dso_local i8 @atomicrmw_xchg_i8_unaligned_acq_rel(ptr %ptr, i8 %value) {
; -O0: subs w8, w8, w10, uxtb
;
; -O1-LABEL: atomicrmw_xchg_i8_unaligned_acq_rel:
-; -O1: ldaxrb w8, [x0]
-; -O1: stlxrb w9, w1, [x0]
+; -O1: ldaxrb w0, [x8]
+; -O1: stlxrb w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i8 %value acq_rel, align 1
ret i8 %r
}
@@ -452,8 +452,8 @@ define dso_local i8 @atomicrmw_xchg_i8_unaligned_seq_cst(ptr %ptr, i8 %value) {
; -O0: subs w8, w8, w10, uxtb
;
; -O1-LABEL: atomicrmw_xchg_i8_unaligned_seq_cst:
-; -O1: ldaxrb w8, [x0]
-; -O1: stlxrb w9, w1, [x0]
+; -O1: ldaxrb w0, [x8]
+; -O1: stlxrb w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i8 %value seq_cst, align 1
ret i8 %r
}
diff --git a/llvm/test/CodeGen/AArch64/Atomics/aarch64-atomicrmw-rcpc3.ll b/llvm/test/CodeGen/AArch64/Atomics/aarch64-atomicrmw-rcpc3.ll
index 725558f2dcf727..28ee1a2a70c4d7 100644
--- a/llvm/test/CodeGen/AArch64/Atomics/aarch64-atomicrmw-rcpc3.ll
+++ b/llvm/test/CodeGen/AArch64/Atomics/aarch64-atomicrmw-rcpc3.ll
@@ -12,8 +12,8 @@ define dso_local i8 @atomicrmw_xchg_i8_aligned_monotonic(ptr %ptr, i8 %value) {
; -O0: subs w8, w8, w10, uxtb
;
; -O1-LABEL: atomicrmw_xchg_i8_aligned_monotonic:
-; -O1: ldxrb w8, [x0]
-; -O1: stxrb w9, w1, [x0]
+; -O1: ldxrb w0, [x8]
+; -O1: stxrb w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i8 %value monotonic, align 1
ret i8 %r
}
@@ -27,8 +27,8 @@ define dso_local i8 @atomicrmw_xchg_i8_aligned_acquire(ptr %ptr, i8 %value) {
; -O0: subs w8, w8, w10, uxtb
;
; -O1-LABEL: atomicrmw_xchg_i8_aligned_acquire:
-; -O1: ldaxrb w8, [x0]
-; -O1: stxrb w9, w1, [x0]
+; -O1: ldaxrb w0, [x8]
+; -O1: stxrb w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i8 %value acquire, align 1
ret i8 %r
}
@@ -42,8 +42,8 @@ define dso_local i8 @atomicrmw_xchg_i8_aligned_release(ptr %ptr, i8 %value) {
; -O0: subs w8, w8, w10, uxtb
;
; -O1-LABEL: atomicrmw_xchg_i8_aligned_release:
-; -O1: ldxrb w8, [x0]
-; -O1: stlxrb w9, w1, [x0]
+; -O1: ldxrb w0, [x8]
+; -O1: stlxrb w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i8 %value release, align 1
ret i8 %r
}
@@ -57,8 +57,8 @@ define dso_local i8 @atomicrmw_xchg_i8_aligned_acq_rel(ptr %ptr, i8 %value) {
; -O0: subs w8, w8, w10, uxtb
;
; -O1-LABEL: atomicrmw_xchg_i8_aligned_acq_rel:
-; -O1: ldaxrb w8, [x0]
-; -O1: stlxrb w9, w1, [x0]
+; -O1: ldaxrb w0, [x8]
+; -O1: stlxrb w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i8 %value acq_rel, align 1
ret i8 %r
}
@@ -72,8 +72,8 @@ define dso_local i8 @atomicrmw_xchg_i8_aligned_seq_cst(ptr %ptr, i8 %value) {
; -O0: subs w8, w8, w10, uxtb
;
; -O1-LABEL: atomicrmw_xchg_i8_aligned_seq_cst:
-; -O1: ldaxrb w8, [x0]
-; -O1: stlxrb w9, w1, [x0]
+; -O1: ldaxrb w0, [x8]
+; -O1: stlxrb w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i8 %value seq_cst, align 1
ret i8 %r
}
@@ -86,8 +86,8 @@ define dso_local i16 @atomicrmw_xchg_i16_aligned_monotonic(ptr %ptr, i16 %value)
; -O0: subs w8, w8, w9, uxth
;
; -O1-LABEL: atomicrmw_xchg_i16_aligned_monotonic:
-; -O1: ldxrh w8, [x0]
-; -O1: stxrh w9, w1, [x0]
+; -O1: ldxrh w0, [x8]
+; -O1: stxrh w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i16 %value monotonic, align 2
ret i16 %r
}
@@ -100,8 +100,8 @@ define dso_local i16 @atomicrmw_xchg_i16_aligned_acquire(ptr %ptr, i16 %value) {
; -O0: subs w8, w8, w9, uxth
;
; -O1-LABEL: atomicrmw_xchg_i16_aligned_acquire:
-; -O1: ldaxrh w8, [x0]
-; -O1: stxrh w9, w1, [x0]
+; -O1: ldaxrh w0, [x8]
+; -O1: stxrh w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i16 %value acquire, align 2
ret i16 %r
}
@@ -114,8 +114,8 @@ define dso_local i16 @atomicrmw_xchg_i16_aligned_release(ptr %ptr, i16 %value) {
; -O0: subs w8, w8, w9, uxth
;
; -O1-LABEL: atomicrmw_xchg_i16_aligned_release:
-; -O1: ldxrh w8, [x0]
-; -O1: stlxrh w9, w1, [x0]
+; -O1: ldxrh w0, [x8]
+; -O1: stlxrh w9, w1, [x8]
%r = atomicrmw xchg ptr %ptr, i16 %value release, align 2
ret i16 %r
}
@@ -128,8 +128,8 @@ define dso_local i16 @atomicrmw_xchg_i16_aligned_acq_rel(ptr %ptr, i16 %value) {
; -O0: subs w8, w8, w9, uxth
;
; -O1-LABEL: atom...
[truncated]
|
qcolombet
reviewed
Feb 4, 2025
|
How bad are the regressions you mentioned? |
arsenm
force-pushed
the
users/arsenm/peephole-opt/fix-looking-for-def-of-current-instruction
branch
from
a897dcf to
84db705
Compare
arsenm
force-pushed
the
users/arsenm/x86/remove-shouldRewriteCopySrc-hack
branch
from
bd710cb to
4b852e6
Compare
Not that bad. Most of the diff is shuffling around register numbers and instruction placement. The differences are 1-2 instructions in most cases. The worst are PPC adding 5+ and one RVV case that happens to hit a worse spill later |
qcolombet
approved these changes
Feb 5, 2025
wangpc-pp
reviewed
Feb 5, 2025
| ; RV32-NEXT: add a0, sp, a0 | ||
| ; RV32-NEXT: addi a0, a0, 48 | ||
| ; RV32-NEXT: vl8r.v v8, (a0) # Unknown-size Folded Reload | ||
| ; RV32-NEXT: vand.vv v16, v24, v8, v0.t | ||
| ; RV32-NEXT: vs8r.v v8, (a0) # Unknown-size Folded Spill |
There was a problem hiding this comment.
For RISCV, this is a regerssion since it requires more spill/reload.
There was a problem hiding this comment.
#120524 and similar may help here
wangpc-pp
reviewed
Feb 5, 2025
| ; RV32-NEXT: mul a1, a1, a2 | ||
| ; RV32-NEXT: add a1, sp, a1 | ||
| ; RV32-NEXT: addi a1, a1, 16 | ||
| ; RV32-NEXT: vl8r.v v0, (a1) # Unknown-size Folded Reload |
| ; RV32-NEXT: add a1, sp, a1 | ||
| ; RV32-NEXT: addi a1, a1, 16 | ||
| ; RV32-NEXT: vl8r.v v16, (a1) # Unknown-size Folded Reload | ||
| ; RV32-NEXT: csrr a1, vlenb | ||
| ; RV32-NEXT: li a3, 76 | ||
| ; RV32-NEXT: li a3, 72 |
There was a problem hiding this comment.
This looks like an improvement.
arsenm
force-pushed
the
users/arsenm/x86/remove-shouldRewriteCopySrc-hack
branch
3 times, most recently
from
d5712fd to
0abe405
Compare
Base automatically changed from
users/arsenm/x86/remove-shouldRewriteCopySrc-hack
to
main
February 5, 2025 16:25
This fixes the handling of subregister extract copies. This will allow AMDGPU to remove its implementation of shouldRewriteCopySrc, which exists as a 10 year old workaround to this bug. peephole-opt-fold-reg-sequence-subreg.mir will show the expected improvement once the custom implementation is removed. The copy coalescing processing here is overly abstracted from what's actually happening. Previously when visiting coalescable copy-like instructions, we would parse the sources one at a time and then pass the def of the root instruction into findNextSource. This means that the first thing the new ValueTracker constructed would do is getVRegDef to find the instruction we are currently processing. This adds an unnecessary step, placing a useless entry in the RewriteMap, and required skipping the no-op case where getNewSource would return the original source operand. This was a problem since in the case of a subregister extract, shouldRewriteCopySource would always say that it is useful to rewrite and the use-def chain walk would abort, returning the original operand. Move the process to start looking at the source operand to begin with. This does not fix the confused handling in the uncoalescable copy case which is proving to be more difficult. Some currently handled cases have multiple defs from a single source, and other handled cases have 0 input operands. It would be simpler if this was implemented with isCopyLikeInstr, rather than guessing at the operand structure as it does now. There are some improvements and some regressions. The regressions appear to be downstream issues for the most part. One of the uglier regressions is in PPC, where a sequence of insert_subrgs is used to build registers. I opened #125502 to use reg_sequence instead, which may help. The worst regression is an absurd SPARC testcase using a <251 x fp128>, which uses a very long chain of insert_subregs. We need improved subregister handling locally in PeepholeOptimizer, and other pasess like MachineCSE to fix some of the other regressions. We should handle subregister composes and folding more indexes into insert_subreg and reg_sequence.
arsenm
force-pushed
the
users/arsenm/peephole-opt/fix-looking-for-def-of-current-instruction
branch
from
84db705 to
0417e7f
Compare
arsenm
deleted the
users/arsenm/peephole-opt/fix-looking-for-def-of-current-instruction
branch
Icohedron
pushed a commit
to Icohedron/llvm-project
that referenced
this pull request
Feb 11, 2025
…5533) This fixes the handling of subregister extract copies. This will allow AMDGPU to remove its implementation of shouldRewriteCopySrc, which exists as a 10 year old workaround to this bug. peephole-opt-fold-reg-sequence-subreg.mir will show the expected improvement once the custom implementation is removed. The copy coalescing processing here is overly abstracted from what's actually happening. Previously when visiting coalescable copy-like instructions, we would parse the sources one at a time and then pass the def of the root instruction into findNextSource. This means that the first thing the new ValueTracker constructed would do is getVRegDef to find the instruction we are currently processing. This adds an unnecessary step, placing a useless entry in the RewriteMap, and required skipping the no-op case where getNewSource would return the original source operand. This was a problem since in the case of a subregister extract, shouldRewriteCopySource would always say that it is useful to rewrite and the use-def chain walk would abort, returning the original operand. Move the process to start looking at the source operand to begin with. This does not fix the confused handling in the uncoalescable copy case which is proving to be more difficult. Some currently handled cases have multiple defs from a single source, and other handled cases have 0 input operands. It would be simpler if this was implemented with isCopyLikeInstr, rather than guessing at the operand structure as it does now. There are some improvements and some regressions. The regressions appear to be downstream issues for the most part. One of the uglier regressions is in PPC, where a sequence of insert_subrgs is used to build registers. I opened llvm#125502 to use reg_sequence instead, which may help. The worst regression is an absurd SPARC testcase using a <251 x fp128>, which uses a very long chain of insert_subregs. We need improved subregister handling locally in PeepholeOptimizer, and other pasess like MachineCSE to fix some of the other regressions. We should handle subregister composes and folding more indexes into insert_subreg and reg_sequence.
lei137
reviewed
Feb 12, 2025
Comment on lines
+37
to
+40
| ; CHECK-NEXT: xxlor vs0, vs4, vs4 | ||
| ; CHECK-NEXT: xxlor vs1, vs5, vs5 | ||
| ; CHECK-NEXT: xxlor vs2, vs6, vs6 | ||
| ; CHECK-NEXT: xxlor vs3, vs7, vs7 |
There was a problem hiding this comment.
These extra copies are a result of using acc1 vs acc0 on line 29. Any idea as to why this patch caused it to use acc1 vs acc0?
There was a problem hiding this comment.
I looked into it a bit and gave up because PPC is using a lot of complex nested custom instruction patterns that will never be handled as nicely as the generic operators. I opened #125502 to start using reg_sequence
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 fixes the handling of subregister extract copies. This
will allow AMDGPU to remove its implementation of
shouldRewriteCopySrc, which exists as a 10 year old workaround
to this bug. peephole-opt-fold-reg-sequence-subreg.mir will
show the expected improvement once the custom implementation
is removed.
The copy coalescing processing here is overly abstracted
from what's actually happening. Previously when visiting
coalescable copy-like instructions, we would parse the
sources one at a time and then pass the def of the root
instruction into findNextSource. This means that the
first thing the new ValueTracker constructed would do
is getVRegDef to find the instruction we are currently
processing. This adds an unnecessary step, placing
a useless entry in the RewriteMap, and required skipping
the no-op case where getNewSource would return the original
source operand. This was a problem since in the case
of a subregister extract, shouldRewriteCopySource would always
say that it is useful to rewrite and the use-def chain walk
would abort, returning the original operand. Move the process
to start looking at the source operand to begin with.
This does not fix the confused handling in the uncoalescable
copy case which is proving to be more difficult. Some currently
handled cases have multiple defs from a single source, and other
handled cases have 0 input operands. It would be simpler if
this was implemented with isCopyLikeInstr, rather than guessing
at the operand structure as it does now.
There are some improvements and some regressions. The
regressions appear to be downstream issues for the most part. One
of the uglier regressions is in PPC, where a sequence of insert_subrgs
is used to build registers. I opened #125502 to use reg_sequence instead,
which may help.
The worst regression is an absurd SPARC testcase using a <251 x fp128>,
which uses a very long chain of insert_subregs.
We need improved subregister handling locally in PeepholeOptimizer,
and other pasess like MachineCSE to fix some of the other regressions.
We should handle subregister composes and folding more indexes
into insert_subreg and reg_sequence.