[MachineCP] Correctly handle register masks and sub-registers #122734

ostannard · 2025-01-13T16:00:04Z

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 should fix the compile time performance regressions which were caused by that. Pre-commit benchmarks, which show that the two performance fixes together restore compile time to the original level: https://llvm-compile-time-tracker.com/?config=Overview&stat=instructions%3Au&remote=ostannard

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.

* 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.

llvmbot · 2025-01-13T16:00:43Z

@llvm/pr-subscribers-llvm-regalloc

@llvm/pr-subscribers-backend-aarch64

Author: Oliver Stannard (ostannard)

Changes

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 should fix the compile time performance regressions which were caused by that. Pre-commit benchmarks, which show that the two performance fixes together restore compile time to the original level: https://llvm-compile-time-tracker.com/?config=Overview&stat=instructions%3Au&remote=ostannard

Full diff: https://github.com/llvm/llvm-project/pull/122734.diff

2 Files Affected:

(modified) llvm/lib/CodeGen/MachineCopyPropagation.cpp (+104-55)
(modified) llvm/test/CodeGen/AArch64/machine-cp-sub-reg.mir (+31-1)

diff --git a/llvm/lib/CodeGen/MachineCopyPropagation.cpp b/llvm/lib/CodeGen/MachineCopyPropagation.cpp
index 49ce4b660c3ae4..382959b745ea14 100644
--- a/llvm/lib/CodeGen/MachineCopyPropagation.cpp
+++ b/llvm/lib/CodeGen/MachineCopyPropagation.cpp
@@ -117,7 +117,32 @@ class CopyTracker {
 
   DenseMap<MCRegUnit, CopyInfo> 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<const uint32_t *, BitVector> 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<MCRegister> Regs,
@@ -164,67 +189,86 @@ 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 = 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.
@@ -960,6 +1004,9 @@ void MachineCopyPropagation::ForwardCopyPropagateBlock(MachineBasicBlock &MBB) {
     // a large set of registers.  Treat clobbered registers the same way as
     // defined registers.
     if (RegMask) {
+      BitVector &PreservedRegUnits =
+          Tracker.getPreservedRegUnits(*RegMask, *TRI);
+
       // Erase any MaybeDeadCopies whose destination register is clobbered.
       for (SmallSetVector<MachineInstr *, 8>::iterator DI =
                MaybeDeadCopies.begin();
@@ -978,9 +1025,11 @@ 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);
+        // 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.
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
+...

bzEq · 2025-01-13T16:08:12Z

llvm/lib/CodeGen/MachineCopyPropagation.cpp

 public:
+  /// Get the set of register units which are preserved by RegMaskOp.
+  BitVector &getPreservedRegUnits(const MachineOperand &RegMaskOp,


Can we use LiveRegUnits instead? LRU should have featured similar functionalities.

Looking into the implementation of LiveRegUnits, it works in a similar way to this, but to re-use it we'd need to create a dummy LRU object for each call, which would probably have worse performance than this. The alternative would be to switch this whole pass over to LiveRegUnits, but that would be a lot of work, and I don't think it would be enough anyway because this pass tracks more than just liveness.

The alternative would be to switch this whole pass over to LiveRegUnits

Yeah, that's the direction we wanna take, so that Matt's concern in #122472 (comment) can be addressed too.

but that would be a lot of work, and I don't think it would be enough anyway because this pass tracks more than just liveness.

That's ok for now.

Every pass tracking physical register liveness should be using LiveRegUnits. Way too many passes are doing their own hand rolled liveness tracking, and buggy

I agree with the general principle, but I'm not sure that would help here, because LiveRegUnits only tracks liveness, but this pass also needs to track other information about the COPY instruction which defined that unit (if any). If that is possible, I don't think I'm qualified to do that re-write, since this is the first time I've looked at this pass. Are you OK with this fix as-is, or do you think that re-write needs to happen first?

I'm fine making progress here, but I'm stating as a general principle we should move every pass over to using LiveRegUnits

bzEq · 2025-01-13T16:09:21Z

llvm/lib/CodeGen/MachineCopyPropagation.cpp

-  /// 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)) {


How about we keep this method and adding a filter BitVector to indicate which regunit should be really clobbered?

I think this way is clearer than adding an optional parameter which changes the behaviour of the existing function.

bzEq

LGTM. Please wait for a couple of days before landing it in case other reviewers have other opinions.

vvereschaka

LGTM

jsji · 2025-02-04T21:02:14Z

llvm/lib/CodeGen/MachineCopyPropagation.cpp

+        // 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))


Looks like this might cause an invalid pointer problem. Some of the RegUnit in CopyInfo is still referencing the MaybeDead using pointer. Then after we call MaybeDead->eraseFromParent(); in following 2 lines, the MI pointer might become invalidated , we will then run into problem in next iteration

I'll take a look at this, do you have a test case which triggers it?

We do, but unfortunately the MIR contains some register class that are downstream only, so we can't provide it now.
The following diff should fix it; however, I don't have reduced MIR available now.

diff --git a/llvm/lib/CodeGen/MachineCopyPropagation.cpp b/llvm/lib/CodeGen/MachineCopyPropagation.cpp index d44b064dcb4b..70dd7d04a153 100644 --- a/llvm/lib/CodeGen/MachineCopyPropagation.cpp +++ b/llvm/lib/CodeGen/MachineCopyPropagation.cpp @@ -1009,18 +1009,30 @@ void MachineCopyPropagation::ForwardCopyPropagateBlock(MachineBasicBlock &MBB) { continue; } - 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)) + bool MIRefedinCopyInfo = false; + for (unsigned RegUnit : TRI->regunits(Reg)) { if (!PreservedRegUnits.test(RegUnit)) Tracker.clobberRegUnit(RegUnit, *TRI, *TII, UseCopyInstr); + else { + if (MaybeDead == Tracker.findCopyForUnit(RegUnit, *TRI)) { + MIRefedinCopyInfo = true; + } + } + } // erase() will return the next valid iterator pointing to the next // element after the erased one. DI = MaybeDeadCopies.erase(DI); + + // Preserved by RegMask, DO NOT remove copy + if (MIRefedinCopyInfo) + continue; + + LLVM_DEBUG(dbgs() << "MCP: Removing copy due to regmask clobbering: "; + MaybeDead->dump()); + MaybeDead->eraseFromParent(); Changed = true; ++NumDeletes;

Great, could you send that as a PR? I think it should be possible to hit this with the ARM floating-point register classes, but I've not managed to work out the details yet.

ostannard added 3 commits January 13, 2025 14:06

Add test showing bug

52c0c66

Fix performance regressions

a5e6e2d

* 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.

ostannard requested review from nikic, bzEq, davemgreen and vvereschaka January 13, 2025 16:00

Restore --verify-machineinstrs

4814c5a

llvmbot added backend:AArch64 llvm:regalloc labels Jan 13, 2025

Delete dead code

6d0ffc0

ostannard mentioned this pull request Jan 13, 2025

[MachineCP] Correctly handle register masks and sub-registers #122472

Merged

bzEq reviewed Jan 13, 2025

View reviewed changes

Merge branch 'main' into machine-cp-reg-mask-perf

c797f06

bzEq approved these changes Jan 15, 2025

View reviewed changes

vvereschaka approved these changes Jan 16, 2025

View reviewed changes

ostannard merged commit 9e436c2 into llvm:main Jan 16, 2025
8 checks passed

jsji reviewed Feb 4, 2025

View reviewed changes

ostannard mentioned this pull request Feb 6, 2025

[LLVM][Clang][AArch64] Implement AArch64 build attributes #123990

Merged

[MachineCP] Correctly handle register masks and sub-registers #122734

[MachineCP] Correctly handle register masks and sub-registers #122734

Uh oh!

Conversation

ostannard commented Jan 13, 2025

Uh oh!

llvmbot commented Jan 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bzEq left a comment

Choose a reason for hiding this comment

Uh oh!

vvereschaka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

llvmbot commented Jan 13, 2025 •

edited

Loading