[MachineCP] Correctly handle register masks and sub-registers #122472
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
When passing an instruction with a register mask, the machine copy propagation pass was dropping the information about some copy instructions which define a register which is preserved by the mask, because that register overlaps a register which is partially clobbered by it. This resulted in a miscompilation for AArch64, because this caused a live copy to be considered dead. The fix is to clobber register masks by finding the set of reg units which is preserved by the mask, and clobbering all units not in that set.
|
@llvm/pr-subscribers-backend-aarch64 Author: Oliver Stannard (ostannard) ChangesWhen passing an instruction with a register mask, the machine copy propagation pass was dropping the information about some copy instructions which define a register which is preserved by the mask, because that register overlaps a register which is partially clobbered by it. This resulted in a miscompilation for AArch64, because this caused a live copy to be considered dead. The fix is to clobber register masks by finding the set of reg units which is preserved by the mask, and clobbering all units not in that set. Full diff: https://github.com/llvm/llvm-project/pull/122472.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/MachineCopyPropagation.cpp b/llvm/lib/CodeGen/MachineCopyPropagation.cpp
index 49ce4b660c3ae4..2168cb51f7df8c 100644
--- a/llvm/lib/CodeGen/MachineCopyPropagation.cpp
+++ b/llvm/lib/CodeGen/MachineCopyPropagation.cpp
@@ -164,67 +164,89 @@ class CopyTracker {
Copies.erase(Unit);
}
- /// Clobber a single register, removing it from the tracker's copy maps.
- void clobberRegister(MCRegister Reg, const TargetRegisterInfo &TRI,
- const TargetInstrInfo &TII, bool UseCopyInstr) {
- for (MCRegUnit Unit : TRI.regunits(Reg)) {
- auto I = Copies.find(Unit);
- if (I != Copies.end()) {
- // When we clobber the source of a copy, we need to clobber everything
- // it defined.
- markRegsUnavailable(I->second.DefRegs, TRI);
- // When we clobber the destination of a copy, we need to clobber the
- // whole register it defined.
- if (MachineInstr *MI = I->second.MI) {
- std::optional<DestSourcePair> CopyOperands =
- isCopyInstr(*MI, TII, UseCopyInstr);
-
- MCRegister Def = CopyOperands->Destination->getReg().asMCReg();
- MCRegister Src = CopyOperands->Source->getReg().asMCReg();
-
- markRegsUnavailable(Def, TRI);
-
- // Since we clobber the destination of a copy, the semantic of Src's
- // "DefRegs" to contain Def is no longer effectual. We will also need
- // to remove the record from the copy maps that indicates Src defined
- // Def. Failing to do so might cause the target to miss some
- // opportunities to further eliminate redundant copy instructions.
- // Consider the following sequence during the
- // ForwardCopyPropagateBlock procedure:
- // L1: r0 = COPY r9 <- TrackMI
- // L2: r0 = COPY r8 <- TrackMI (Remove r9 defined r0 from tracker)
- // L3: use r0 <- Remove L2 from MaybeDeadCopies
- // L4: early-clobber r9 <- Clobber r9 (L2 is still valid in tracker)
- // L5: r0 = COPY r8 <- Remove NopCopy
- for (MCRegUnit SrcUnit : TRI.regunits(Src)) {
- auto SrcCopy = Copies.find(SrcUnit);
- if (SrcCopy != Copies.end() && SrcCopy->second.LastSeenUseInCopy) {
- // If SrcCopy defines multiple values, we only need
- // to erase the record for Def in DefRegs.
- for (auto itr = SrcCopy->second.DefRegs.begin();
- itr != SrcCopy->second.DefRegs.end(); itr++) {
- if (*itr == Def) {
- SrcCopy->second.DefRegs.erase(itr);
- // If DefReg becomes empty after removal, we can remove the
- // SrcCopy from the tracker's copy maps. We only remove those
- // entries solely record the Def is defined by Src. If an
- // entry also contains the definition record of other Def'
- // registers, it cannot be cleared.
- if (SrcCopy->second.DefRegs.empty() && !SrcCopy->second.MI) {
- Copies.erase(SrcCopy);
- }
- break;
+ /// Clobber a single register unit, removing it from the tracker's copy maps.
+ void clobberRegUnit(MCRegUnit Unit, const TargetRegisterInfo &TRI,
+ const TargetInstrInfo &TII, bool UseCopyInstr) {
+ auto I = Copies.find(Unit);
+ if (I != Copies.end()) {
+ // When we clobber the source of a copy, we need to clobber everything
+ // it defined.
+ markRegsUnavailable(I->second.DefRegs, TRI);
+ // When we clobber the destination of a copy, we need to clobber the
+ // whole register it defined.
+ if (MachineInstr *MI = I->second.MI) {
+ std::optional<DestSourcePair> CopyOperands =
+ isCopyInstr(*MI, TII, UseCopyInstr);
+
+ MCRegister Def = CopyOperands->Destination->getReg().asMCReg();
+ MCRegister Src = CopyOperands->Source->getReg().asMCReg();
+
+ markRegsUnavailable(Def, TRI);
+
+ // Since we clobber the destination of a copy, the semantic of Src's
+ // "DefRegs" to contain Def is no longer effectual. We will also need
+ // to remove the record from the copy maps that indicates Src defined
+ // Def. Failing to do so might cause the target to miss some
+ // opportunities to further eliminate redundant copy instructions.
+ // Consider the following sequence during the
+ // ForwardCopyPropagateBlock procedure:
+ // L1: r0 = COPY r9 <- TrackMI
+ // L2: r0 = COPY r8 <- TrackMI (Remove r9 defined r0 from tracker)
+ // L3: use r0 <- Remove L2 from MaybeDeadCopies
+ // L4: early-clobber r9 <- Clobber r9 (L2 is still valid in tracker)
+ // L5: r0 = COPY r8 <- Remove NopCopy
+ for (MCRegUnit SrcUnit : TRI.regunits(Src)) {
+ auto SrcCopy = Copies.find(SrcUnit);
+ if (SrcCopy != Copies.end() && SrcCopy->second.LastSeenUseInCopy) {
+ // If SrcCopy defines multiple values, we only need
+ // to erase the record for Def in DefRegs.
+ for (auto itr = SrcCopy->second.DefRegs.begin();
+ itr != SrcCopy->second.DefRegs.end(); itr++) {
+ if (*itr == Def) {
+ SrcCopy->second.DefRegs.erase(itr);
+ // If DefReg becomes empty after removal, we can remove the
+ // SrcCopy from the tracker's copy maps. We only remove those
+ // entries solely record the Def is defined by Src. If an
+ // entry also contains the definition record of other Def'
+ // registers, it cannot be cleared.
+ if (SrcCopy->second.DefRegs.empty() && !SrcCopy->second.MI) {
+ Copies.erase(SrcCopy);
}
+ break;
}
}
}
}
- // Now we can erase the copy.
- Copies.erase(I);
}
+ // Now we can erase the copy.
+ Copies.erase(I);
}
}
+ /// Clobber a single register, removing it from the tracker's copy maps.
+ void clobberRegister(MCRegister Reg, const TargetRegisterInfo &TRI,
+ const TargetInstrInfo &TII, bool UseCopyInstr) {
+ for (MCRegUnit Unit : TRI.regunits(Reg)) {
+ clobberRegUnit(Unit, TRI, TII, UseCopyInstr);
+ }
+ }
+
+ /// Clobber all registers which are not preserved by RegMask, removing them
+ /// from the tracker's copy maps.
+ void clobberRegistersExceptMask(const MachineOperand *RegMask, const TargetRegisterInfo &TRI,
+ const TargetInstrInfo &TII, bool UseCopyInstr) {
+ BitVector SafeRegUnits(TRI.getNumRegUnits());
+
+ for (unsigned SafeReg = 0, E = TRI.getNumRegs(); SafeReg < E; ++SafeReg)
+ if (!RegMask->clobbersPhysReg(SafeReg))
+ for (auto SafeUnit : TRI.regunits(SafeReg))
+ SafeRegUnits.set(SafeUnit);
+
+ for (unsigned Unit = 0, E = TRI.getNumRegUnits(); Unit < E; ++Unit)
+ if (!SafeRegUnits.test(Unit))
+ clobberRegUnit(Unit, TRI, TII, UseCopyInstr);
+ }
+
/// Track copy's src users, and return false if that can't be done.
/// We can only track if we have a COPY instruction which source is
/// the same as the Reg.
@@ -960,6 +982,10 @@ void MachineCopyPropagation::ForwardCopyPropagateBlock(MachineBasicBlock &MBB) {
// a large set of registers. Treat clobbered registers the same way as
// defined registers.
if (RegMask) {
+ // Invalidate all entries in the copy map which are not preserved by this
+ // register mask.
+ Tracker.clobberRegistersExceptMask(RegMask, *TRI, *TII, UseCopyInstr);
+
// Erase any MaybeDeadCopies whose destination register is clobbered.
for (SmallSetVector<MachineInstr *, 8>::iterator DI =
MaybeDeadCopies.begin();
@@ -978,10 +1004,6 @@ void MachineCopyPropagation::ForwardCopyPropagateBlock(MachineBasicBlock &MBB) {
LLVM_DEBUG(dbgs() << "MCP: Removing copy due to regmask clobbering: ";
MaybeDead->dump());
- // Make sure we invalidate any entries in the copy maps before erasing
- // the instruction.
- Tracker.clobberRegister(Reg, *TRI, *TII, UseCopyInstr);
-
// erase() will return the next valid iterator pointing to the next
// element after the erased one.
DI = MaybeDeadCopies.erase(DI);
diff --git a/llvm/test/CodeGen/AArch64/machine-cp-sub-reg.mir b/llvm/test/CodeGen/AArch64/machine-cp-sub-reg.mir
index 5b379c2bd56292..e7865569c75bd0 100644
--- a/llvm/test/CodeGen/AArch64/machine-cp-sub-reg.mir
+++ b/llvm/test/CodeGen/AArch64/machine-cp-sub-reg.mir
@@ -1,5 +1,16 @@
# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 4
-# RUN: llc -o - %s --run-pass=machine-cp -mcp-use-is-copy-instr -mtriple=arm64-apple-macos --verify-machineinstrs | FileCheck %s
+# RUN: llc -o - %s --run-pass=machine-cp -mcp-use-is-copy-instr -mtriple=arm64-apple-macos | FileCheck %s
+
+--- |
+ declare void @foo()
+
+ define void @test() {
+ unreachable
+ }
+ define void @test2() {
+ unreachable
+ }
+...
---
name: test
@@ -30,3 +41,22 @@ body: |
RET undef $lr, implicit $x0
...
+---
+name: test2
+tracksRegLiveness: true
+body: |
+ bb.0:
+ liveins: $q14, $d29, $x0, $x1
+ ; CHECK-LABEL: name: test2
+ ; CHECK: liveins: $q14, $d29, $x0, $x1
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: renamable $d8 = COPY killed renamable $d29
+ ; CHECK-NEXT: BL @foo, csr_aarch64_aapcs, implicit-def dead $lr, implicit $sp, implicit-def $sp
+ ; CHECK-NEXT: renamable $b0 = SMAXVv8i8v killed renamable $d8, implicit-def $q0
+ ; CHECK-NEXT: RET_ReallyLR implicit $b0
+ renamable $q8 = COPY renamable $q14
+ renamable $d8 = COPY killed renamable $d29
+ BL @foo, csr_aarch64_aapcs, implicit-def dead $lr, implicit $sp, implicit-def $sp
+ renamable $b0 = SMAXVv8i8v killed renamable $d8, implicit-def $q0
+ RET_ReallyLR implicit $b0
+...
|
|
@llvm/pr-subscribers-llvm-regalloc Author: Oliver Stannard (ostannard) ChangesWhen passing an instruction with a register mask, the machine copy propagation pass was dropping the information about some copy instructions which define a register which is preserved by the mask, because that register overlaps a register which is partially clobbered by it. This resulted in a miscompilation for AArch64, because this caused a live copy to be considered dead. The fix is to clobber register masks by finding the set of reg units which is preserved by the mask, and clobbering all units not in that set. Full diff: https://github.com/llvm/llvm-project/pull/122472.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/MachineCopyPropagation.cpp b/llvm/lib/CodeGen/MachineCopyPropagation.cpp
index 49ce4b660c3ae4..2168cb51f7df8c 100644
--- a/llvm/lib/CodeGen/MachineCopyPropagation.cpp
+++ b/llvm/lib/CodeGen/MachineCopyPropagation.cpp
@@ -164,67 +164,89 @@ class CopyTracker {
Copies.erase(Unit);
}
- /// Clobber a single register, removing it from the tracker's copy maps.
- void clobberRegister(MCRegister Reg, const TargetRegisterInfo &TRI,
- const TargetInstrInfo &TII, bool UseCopyInstr) {
- for (MCRegUnit Unit : TRI.regunits(Reg)) {
- auto I = Copies.find(Unit);
- if (I != Copies.end()) {
- // When we clobber the source of a copy, we need to clobber everything
- // it defined.
- markRegsUnavailable(I->second.DefRegs, TRI);
- // When we clobber the destination of a copy, we need to clobber the
- // whole register it defined.
- if (MachineInstr *MI = I->second.MI) {
- std::optional<DestSourcePair> CopyOperands =
- isCopyInstr(*MI, TII, UseCopyInstr);
-
- MCRegister Def = CopyOperands->Destination->getReg().asMCReg();
- MCRegister Src = CopyOperands->Source->getReg().asMCReg();
-
- markRegsUnavailable(Def, TRI);
-
- // Since we clobber the destination of a copy, the semantic of Src's
- // "DefRegs" to contain Def is no longer effectual. We will also need
- // to remove the record from the copy maps that indicates Src defined
- // Def. Failing to do so might cause the target to miss some
- // opportunities to further eliminate redundant copy instructions.
- // Consider the following sequence during the
- // ForwardCopyPropagateBlock procedure:
- // L1: r0 = COPY r9 <- TrackMI
- // L2: r0 = COPY r8 <- TrackMI (Remove r9 defined r0 from tracker)
- // L3: use r0 <- Remove L2 from MaybeDeadCopies
- // L4: early-clobber r9 <- Clobber r9 (L2 is still valid in tracker)
- // L5: r0 = COPY r8 <- Remove NopCopy
- for (MCRegUnit SrcUnit : TRI.regunits(Src)) {
- auto SrcCopy = Copies.find(SrcUnit);
- if (SrcCopy != Copies.end() && SrcCopy->second.LastSeenUseInCopy) {
- // If SrcCopy defines multiple values, we only need
- // to erase the record for Def in DefRegs.
- for (auto itr = SrcCopy->second.DefRegs.begin();
- itr != SrcCopy->second.DefRegs.end(); itr++) {
- if (*itr == Def) {
- SrcCopy->second.DefRegs.erase(itr);
- // If DefReg becomes empty after removal, we can remove the
- // SrcCopy from the tracker's copy maps. We only remove those
- // entries solely record the Def is defined by Src. If an
- // entry also contains the definition record of other Def'
- // registers, it cannot be cleared.
- if (SrcCopy->second.DefRegs.empty() && !SrcCopy->second.MI) {
- Copies.erase(SrcCopy);
- }
- break;
+ /// Clobber a single register unit, removing it from the tracker's copy maps.
+ void clobberRegUnit(MCRegUnit Unit, const TargetRegisterInfo &TRI,
+ const TargetInstrInfo &TII, bool UseCopyInstr) {
+ auto I = Copies.find(Unit);
+ if (I != Copies.end()) {
+ // When we clobber the source of a copy, we need to clobber everything
+ // it defined.
+ markRegsUnavailable(I->second.DefRegs, TRI);
+ // When we clobber the destination of a copy, we need to clobber the
+ // whole register it defined.
+ if (MachineInstr *MI = I->second.MI) {
+ std::optional<DestSourcePair> CopyOperands =
+ isCopyInstr(*MI, TII, UseCopyInstr);
+
+ MCRegister Def = CopyOperands->Destination->getReg().asMCReg();
+ MCRegister Src = CopyOperands->Source->getReg().asMCReg();
+
+ markRegsUnavailable(Def, TRI);
+
+ // Since we clobber the destination of a copy, the semantic of Src's
+ // "DefRegs" to contain Def is no longer effectual. We will also need
+ // to remove the record from the copy maps that indicates Src defined
+ // Def. Failing to do so might cause the target to miss some
+ // opportunities to further eliminate redundant copy instructions.
+ // Consider the following sequence during the
+ // ForwardCopyPropagateBlock procedure:
+ // L1: r0 = COPY r9 <- TrackMI
+ // L2: r0 = COPY r8 <- TrackMI (Remove r9 defined r0 from tracker)
+ // L3: use r0 <- Remove L2 from MaybeDeadCopies
+ // L4: early-clobber r9 <- Clobber r9 (L2 is still valid in tracker)
+ // L5: r0 = COPY r8 <- Remove NopCopy
+ for (MCRegUnit SrcUnit : TRI.regunits(Src)) {
+ auto SrcCopy = Copies.find(SrcUnit);
+ if (SrcCopy != Copies.end() && SrcCopy->second.LastSeenUseInCopy) {
+ // If SrcCopy defines multiple values, we only need
+ // to erase the record for Def in DefRegs.
+ for (auto itr = SrcCopy->second.DefRegs.begin();
+ itr != SrcCopy->second.DefRegs.end(); itr++) {
+ if (*itr == Def) {
+ SrcCopy->second.DefRegs.erase(itr);
+ // If DefReg becomes empty after removal, we can remove the
+ // SrcCopy from the tracker's copy maps. We only remove those
+ // entries solely record the Def is defined by Src. If an
+ // entry also contains the definition record of other Def'
+ // registers, it cannot be cleared.
+ if (SrcCopy->second.DefRegs.empty() && !SrcCopy->second.MI) {
+ Copies.erase(SrcCopy);
}
+ break;
}
}
}
}
- // Now we can erase the copy.
- Copies.erase(I);
}
+ // Now we can erase the copy.
+ Copies.erase(I);
}
}
+ /// Clobber a single register, removing it from the tracker's copy maps.
+ void clobberRegister(MCRegister Reg, const TargetRegisterInfo &TRI,
+ const TargetInstrInfo &TII, bool UseCopyInstr) {
+ for (MCRegUnit Unit : TRI.regunits(Reg)) {
+ clobberRegUnit(Unit, TRI, TII, UseCopyInstr);
+ }
+ }
+
+ /// Clobber all registers which are not preserved by RegMask, removing them
+ /// from the tracker's copy maps.
+ void clobberRegistersExceptMask(const MachineOperand *RegMask, const TargetRegisterInfo &TRI,
+ const TargetInstrInfo &TII, bool UseCopyInstr) {
+ BitVector SafeRegUnits(TRI.getNumRegUnits());
+
+ for (unsigned SafeReg = 0, E = TRI.getNumRegs(); SafeReg < E; ++SafeReg)
+ if (!RegMask->clobbersPhysReg(SafeReg))
+ for (auto SafeUnit : TRI.regunits(SafeReg))
+ SafeRegUnits.set(SafeUnit);
+
+ for (unsigned Unit = 0, E = TRI.getNumRegUnits(); Unit < E; ++Unit)
+ if (!SafeRegUnits.test(Unit))
+ clobberRegUnit(Unit, TRI, TII, UseCopyInstr);
+ }
+
/// Track copy's src users, and return false if that can't be done.
/// We can only track if we have a COPY instruction which source is
/// the same as the Reg.
@@ -960,6 +982,10 @@ void MachineCopyPropagation::ForwardCopyPropagateBlock(MachineBasicBlock &MBB) {
// a large set of registers. Treat clobbered registers the same way as
// defined registers.
if (RegMask) {
+ // Invalidate all entries in the copy map which are not preserved by this
+ // register mask.
+ Tracker.clobberRegistersExceptMask(RegMask, *TRI, *TII, UseCopyInstr);
+
// Erase any MaybeDeadCopies whose destination register is clobbered.
for (SmallSetVector<MachineInstr *, 8>::iterator DI =
MaybeDeadCopies.begin();
@@ -978,10 +1004,6 @@ void MachineCopyPropagation::ForwardCopyPropagateBlock(MachineBasicBlock &MBB) {
LLVM_DEBUG(dbgs() << "MCP: Removing copy due to regmask clobbering: ";
MaybeDead->dump());
- // Make sure we invalidate any entries in the copy maps before erasing
- // the instruction.
- Tracker.clobberRegister(Reg, *TRI, *TII, UseCopyInstr);
-
// erase() will return the next valid iterator pointing to the next
// element after the erased one.
DI = MaybeDeadCopies.erase(DI);
diff --git a/llvm/test/CodeGen/AArch64/machine-cp-sub-reg.mir b/llvm/test/CodeGen/AArch64/machine-cp-sub-reg.mir
index 5b379c2bd56292..e7865569c75bd0 100644
--- a/llvm/test/CodeGen/AArch64/machine-cp-sub-reg.mir
+++ b/llvm/test/CodeGen/AArch64/machine-cp-sub-reg.mir
@@ -1,5 +1,16 @@
# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 4
-# RUN: llc -o - %s --run-pass=machine-cp -mcp-use-is-copy-instr -mtriple=arm64-apple-macos --verify-machineinstrs | FileCheck %s
+# RUN: llc -o - %s --run-pass=machine-cp -mcp-use-is-copy-instr -mtriple=arm64-apple-macos | FileCheck %s
+
+--- |
+ declare void @foo()
+
+ define void @test() {
+ unreachable
+ }
+ define void @test2() {
+ unreachable
+ }
+...
---
name: test
@@ -30,3 +41,22 @@ body: |
RET undef $lr, implicit $x0
...
+---
+name: test2
+tracksRegLiveness: true
+body: |
+ bb.0:
+ liveins: $q14, $d29, $x0, $x1
+ ; CHECK-LABEL: name: test2
+ ; CHECK: liveins: $q14, $d29, $x0, $x1
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: renamable $d8 = COPY killed renamable $d29
+ ; CHECK-NEXT: BL @foo, csr_aarch64_aapcs, implicit-def dead $lr, implicit $sp, implicit-def $sp
+ ; CHECK-NEXT: renamable $b0 = SMAXVv8i8v killed renamable $d8, implicit-def $q0
+ ; CHECK-NEXT: RET_ReallyLR implicit $b0
+ renamable $q8 = COPY renamable $q14
+ renamable $d8 = COPY killed renamable $d29
+ BL @foo, csr_aarch64_aapcs, implicit-def dead $lr, implicit $sp, implicit-def $sp
+ renamable $b0 = SMAXVv8i8v killed renamable $d8, implicit-def $q0
+ RET_ReallyLR implicit $b0
+...
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
vvereschaka
approved these changes
Jan 11, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/187/builds/3710 Here is the relevant piece of the build log for the reference |
|
That test has been failing on that bot intermittently for the last few days, so ignoring it. |
|
I've reverted this in d6f7f2a because it causes a large compile-time regression, see https://llvm-compile-time-tracker.com/compare.php?from=d03f35f9b6d031d6a9375d90ccf7cc285f8e4b79&to=e2a071ece58790f8dd4886e998033cab82e906fb&stat=instructions:u. It should be possible to do this more efficiently. |
|
To avoid checking all regunits, is there anything we can do in |
|
Can you also illustrate what happens leading to the miscompilation? |
I'm not sure what could be done there, can you elaborate? My current ideas are to reduce the number of register units we need to check by iterating over the keys of either
The first commit inside this PR adds an MIR test which shows the bug happening. What's happening is that there is a copy to a Q register, followed by a copy to the overlapping D register. Then there is a function call, which preserves the D register, but not the top half of the overlapping Q register. The D register is used after the call, but machine copy propagation was removing both of those copies from the CopyTracker, not just the Q register one, so it though that the D register copy was dead too. I also have a C reproducer (fuzzer-generated, using lots of vector intrinsics), but it's hundreds of lines long and I haven't been able to reduce it any further. |
bzEq
reviewed
Jan 13, 2025
| // Invalidate all entries in the copy map which are not preserved by this | ||
| // register mask. | ||
| Tracker.clobberRegistersExceptMask(RegMask, *TRI, *TII, UseCopyInstr); | ||
|
|
There was a problem hiding this comment.
At L973, can we get precise RegUnits clobbered by the RegMask? Thus we change L983 to
Tracker.clobberRegUnits(<regunits-returned-at-L973>)
There was a problem hiding this comment.
A RegMask is a set of registers (not register units) which are preserved, and I can't see any way to convert that to a set of register units which are clobbered faster than building the full set of preserved/clobbered register units, which is what the first version of this did.
There was a problem hiding this comment.
We should eventually stop tracking register masks and live in registers, and migrate all infrastructure to work in terms of regunits. There is a cost to mapping between the two formats everywhere
|
Follow-up with the performance fix: #122734 |
kazutakahirata
pushed a commit
to kazutakahirata/llvm-project
that referenced
this pull request
Jan 13, 2025
…22472) When passing an instruction with a register mask, the machine copy propagation pass was dropping the information about some copy instructions which define a register which is preserved by the mask, because that register overlaps a register which is partially clobbered by it. This resulted in a miscompilation for AArch64, because this caused a live copy to be considered dead. The fix is to clobber register masks by finding the set of reg units which is preserved by the mask, and clobbering all units not in that set.
kazutakahirata
pushed a commit
to kazutakahirata/llvm-project
that referenced
this pull request
Jan 13, 2025
…llvm#122472)" This reverts commit e2a071e. This causes a large compile-time regression.
ostannard
added a commit
that referenced
this pull request
Jan 16, 2025
When passing an instruction with a register mask, the machine copy propagation pass was dropping the information about some copy instructions which define a register which is preserved by the mask, because that register overlaps a register which is partially clobbered by it. This resulted in a miscompilation for AArch64, because this caused a live copy to be considered dead. The fix is to clobber register masks by finding the set of reg units which is preserved by the mask, and clobbering all units not in that set. This is based on #122472, and fixes the compile time performance regressions which were caused by that.
DKLoehr
pushed a commit
to DKLoehr/llvm-project
that referenced
this pull request
Jan 17, 2025
…22472) When passing an instruction with a register mask, the machine copy propagation pass was dropping the information about some copy instructions which define a register which is preserved by the mask, because that register overlaps a register which is partially clobbered by it. This resulted in a miscompilation for AArch64, because this caused a live copy to be considered dead. The fix is to clobber register masks by finding the set of reg units which is preserved by the mask, and clobbering all units not in that set.
DKLoehr
pushed a commit
to DKLoehr/llvm-project
that referenced
this pull request
Jan 17, 2025
…llvm#122472)" This reverts commit e2a071e. This causes a large compile-time regression.
DKLoehr
pushed a commit
to DKLoehr/llvm-project
that referenced
this pull request
Jan 17, 2025
…22734) When passing an instruction with a register mask, the machine copy propagation pass was dropping the information about some copy instructions which define a register which is preserved by the mask, because that register overlaps a register which is partially clobbered by it. This resulted in a miscompilation for AArch64, because this caused a live copy to be considered dead. The fix is to clobber register masks by finding the set of reg units which is preserved by the mask, and clobbering all units not in that set. This is based on llvm#122472, and fixes the compile time performance regressions which were caused by that.
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 passing an instruction with a register mask, the machine copy propagation pass was dropping the information about some copy instructions which define a register which is preserved by the mask, because that register overlaps a register which is partially clobbered by it. This resulted in a miscompilation for AArch64, because this caused a live copy to be considered dead.
The fix is to clobber register masks by finding the set of reg units which is preserved by the mask, and clobbering all units not in that set.