From 52c0c66acf237d979c464be8261063ec8d81881e Mon Sep 17 00:00:00 2001 From: Oliver Stannard Date: Fri, 10 Jan 2025 14:39:56 +0000 Subject: [PATCH 1/5] Add test showing bug --- .../CodeGen/AArch64/machine-cp-sub-reg.mir | 31 ++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/llvm/test/CodeGen/AArch64/machine-cp-sub-reg.mir b/llvm/test/CodeGen/AArch64/machine-cp-sub-reg.mir index 5b379c2bd5629..59aadbe4215c0 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,21 @@ 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: 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 +... From 75fff782526882dfabf5853d3ceaf18adef9e3ac Mon Sep 17 00:00:00 2001 From: Oliver Stannard Date: Fri, 10 Jan 2025 14:47:53 +0000 Subject: [PATCH 2/5] [MachineCP] Correctly handle register masks and sub-registers 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/lib/CodeGen/MachineCopyPropagation.cpp | 136 ++++++++++-------- .../CodeGen/AArch64/machine-cp-sub-reg.mir | 1 + 2 files changed, 81 insertions(+), 56 deletions(-) diff --git a/llvm/lib/CodeGen/MachineCopyPropagation.cpp b/llvm/lib/CodeGen/MachineCopyPropagation.cpp index 49ce4b660c3ae..d2579e2d1b44c 100644 --- a/llvm/lib/CodeGen/MachineCopyPropagation.cpp +++ b/llvm/lib/CodeGen/MachineCopyPropagation.cpp @@ -164,67 +164,91 @@ 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 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 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 +984,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::iterator DI = MaybeDeadCopies.begin(); @@ -978,10 +1006,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 59aadbe4215c0..e7865569c75bd 100644 --- a/llvm/test/CodeGen/AArch64/machine-cp-sub-reg.mir +++ b/llvm/test/CodeGen/AArch64/machine-cp-sub-reg.mir @@ -50,6 +50,7 @@ body: | ; 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 From a5e6e2da61040611849459e92a9fe7e2f548dc6b Mon Sep 17 00:00:00 2001 From: Oliver Stannard Date: Mon, 13 Jan 2025 14:36:46 +0000 Subject: [PATCH 3/5] Fix performance regressions * Memoise the construction of the set of register units preserved by a register mask. * Only check register units covered by a copy being tracked in MaybeDeadCopies. --- llvm/lib/CodeGen/MachineCopyPropagation.cpp | 43 ++++++++++++++++----- 1 file changed, 34 insertions(+), 9 deletions(-) diff --git a/llvm/lib/CodeGen/MachineCopyPropagation.cpp b/llvm/lib/CodeGen/MachineCopyPropagation.cpp index d2579e2d1b44c..382959b745ea1 100644 --- a/llvm/lib/CodeGen/MachineCopyPropagation.cpp +++ b/llvm/lib/CodeGen/MachineCopyPropagation.cpp @@ -117,7 +117,32 @@ class CopyTracker { DenseMap Copies; + // Memoised sets of register units which are preserved by each register mask, + // needed to efficiently remove copies which are invalidated by call + // instructions. + DenseMap RegMaskToPreservedRegUnits; + public: + /// Get the set of register units which are preserved by RegMaskOp. + BitVector &getPreservedRegUnits(const MachineOperand &RegMaskOp, + const TargetRegisterInfo &TRI) { + const uint32_t *RegMask = RegMaskOp.getRegMask(); + auto Existing = RegMaskToPreservedRegUnits.find(RegMask); + if (Existing != RegMaskToPreservedRegUnits.end()) { + return Existing->second; + } else { + BitVector &PreservedRegUnits = RegMaskToPreservedRegUnits[RegMask]; + + PreservedRegUnits.resize(TRI.getNumRegUnits()); + for (unsigned SafeReg = 0, E = TRI.getNumRegs(); SafeReg < E; ++SafeReg) + if (!RegMaskOp.clobbersPhysReg(SafeReg)) + for (auto SafeUnit : TRI.regunits(SafeReg)) + PreservedRegUnits.set(SafeUnit); + + return PreservedRegUnits; + } + } + /// Mark all of the given registers and their subregisters as unavailable for /// copying. void markRegsUnavailable(ArrayRef Regs, @@ -237,12 +262,7 @@ class CopyTracker { 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); + BitVector &SafeRegUnits = getPreservedRegUnits(*RegMask, TRI); for (unsigned Unit = 0, E = TRI.getNumRegUnits(); Unit < E; ++Unit) if (!SafeRegUnits.test(Unit)) @@ -984,9 +1004,8 @@ 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); + BitVector &PreservedRegUnits = + Tracker.getPreservedRegUnits(*RegMask, *TRI); // Erase any MaybeDeadCopies whose destination register is clobbered. for (SmallSetVector::iterator DI = @@ -1006,6 +1025,12 @@ void MachineCopyPropagation::ForwardCopyPropagateBlock(MachineBasicBlock &MBB) { LLVM_DEBUG(dbgs() << "MCP: Removing copy due to regmask clobbering: "; MaybeDead->dump()); + // Invalidate all entries in the copy map which are not preserved by + // this register mask. + for (unsigned RegUnit : TRI->regunits(Reg)) + if (!PreservedRegUnits.test(RegUnit)) + Tracker.clobberRegUnit(RegUnit, *TRI, *TII, UseCopyInstr); + // erase() will return the next valid iterator pointing to the next // element after the erased one. DI = MaybeDeadCopies.erase(DI); From 4814c5afca1c6c900cca1a4ac57e76fe4e19a673 Mon Sep 17 00:00:00 2001 From: Oliver Stannard Date: Mon, 13 Jan 2025 16:00:19 +0000 Subject: [PATCH 4/5] Restore --verify-machineinstrs --- llvm/test/CodeGen/AArch64/machine-cp-sub-reg.mir | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/llvm/test/CodeGen/AArch64/machine-cp-sub-reg.mir b/llvm/test/CodeGen/AArch64/machine-cp-sub-reg.mir index e7865569c75bd..c166b6b48f981 100644 --- a/llvm/test/CodeGen/AArch64/machine-cp-sub-reg.mir +++ b/llvm/test/CodeGen/AArch64/machine-cp-sub-reg.mir @@ -1,5 +1,5 @@ # 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 | FileCheck %s +# RUN: llc -o - %s --run-pass=machine-cp -mcp-use-is-copy-instr -mtriple=arm64-apple-macos --verify-machineinstrs | FileCheck %s --- | declare void @foo() From 6d0ffc0d06030d61d55aa1bb53813712360d75b7 Mon Sep 17 00:00:00 2001 From: Oliver Stannard Date: Mon, 13 Jan 2025 16:00:53 +0000 Subject: [PATCH 5/5] Delete dead code --- llvm/lib/CodeGen/MachineCopyPropagation.cpp | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/llvm/lib/CodeGen/MachineCopyPropagation.cpp b/llvm/lib/CodeGen/MachineCopyPropagation.cpp index 382959b745ea1..0afd73d8ecdcc 100644 --- a/llvm/lib/CodeGen/MachineCopyPropagation.cpp +++ b/llvm/lib/CodeGen/MachineCopyPropagation.cpp @@ -256,19 +256,6 @@ class CopyTracker { } } - /// 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 = getPreservedRegUnits(*RegMask, TRI); - - 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.