[CodeGen] Correctly handle non-standard cases in RemoveLoadsIntoFakeUses (#111551)

SLTozer · web-flow · commit 22687aa97bda · 2025-01-28T13:59:41.000Z
In the RemoveLoadsIntoFakeUses pass, we try to remove loads that are
only used by fake uses, as well as the fake use in question. There are
two existing errors with the pass however: it incorrectly examines every
operand of each FAKE_USE, when only the first is relevant (extra
operands will just be "killed" regs assigned by a previous pass), and it
ignores cases where the FAKE_USE register is not an exact match for the
loaded register, which is incorrect as regalloc may choose to load a
wider value than the FAKE_USE required pre-regalloc. This patch fixes
both of these cases.
diff --git a/llvm/lib/CodeGen/RemoveLoadsIntoFakeUses.cpp b/llvm/lib/CodeGen/RemoveLoadsIntoFakeUses.cpp
@@ -32,6 +32,7 @@
 #include "llvm/IR/Function.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/Support/Debug.h"
+#include "llvm/Target/TargetMachine.h"
 
 using namespace llvm;
 
@@ -74,6 +75,10 @@ INITIALIZE_PASS_END(RemoveLoadsIntoFakeUses, DEBUG_TYPE,
                     "Remove Loads Into Fake Uses", false, false)
 
 bool RemoveLoadsIntoFakeUses::runOnMachineFunction(MachineFunction &MF) {
+  // Skip this pass if we would use VarLoc-based LDV, as there may be DBG_VALUE
+  // instructions of the restored values that would become invalid.
+  if (!MF.useDebugInstrRef())
+    return false;
   // Only run this for functions that have fake uses.
   if (!MF.hasFakeUses() || skipFunction(MF.getFunction()))
     return false;
@@ -86,20 +91,20 @@ bool RemoveLoadsIntoFakeUses::runOnMachineFunction(MachineFunction &MF) {
   const TargetInstrInfo *TII = ST.getInstrInfo();
   const TargetRegisterInfo *TRI = ST.getRegisterInfo();
 
-  SmallDenseMap<Register, SmallVector<MachineInstr *>> RegFakeUses;
+  SmallVector<MachineInstr *> RegFakeUses;
   LivePhysRegs.init(*TRI);
   SmallVector<MachineInstr *, 16> Statepoints;
   for (MachineBasicBlock *MBB : post_order(&MF)) {
+    RegFakeUses.clear();
     LivePhysRegs.addLiveOuts(*MBB);
 
     for (MachineInstr &MI : make_early_inc_range(reverse(*MBB))) {
       if (MI.isFakeUse()) {
-        for (const MachineOperand &MO : MI.operands()) {
-          // Track the Fake Uses that use this register so that we can delete
-          // them if we delete the corresponding load.
-          if (MO.isReg())
-            RegFakeUses[MO.getReg()].push_back(&MI);
-        }
+        if (MI.getNumOperands() == 0 || !MI.getOperand(0).isReg())
+          continue;
+        // Track the Fake Uses that use these register units so that we can
+        // delete them if we delete the corresponding load.
+        RegFakeUses.push_back(&MI);
         // Do not record FAKE_USE uses in LivePhysRegs so that we can recognize
         // otherwise-unused loads.
         continue;
@@ -109,31 +114,38 @@ bool RemoveLoadsIntoFakeUses::runOnMachineFunction(MachineFunction &MF) {
       // reload of a spilled register.
       if (MI.getRestoreSize(TII)) {
         Register Reg = MI.getOperand(0).getReg();
-        assert(Reg.isPhysical() && "VReg seen in function with NoVRegs set?");
         // Don't delete live physreg defs, or any reserved register defs.
         if (!LivePhysRegs.available(Reg) || MRI->isReserved(Reg))
           continue;
-        // There should be an exact match between the loaded register and the
-        // FAKE_USE use. If not, this is a load that is unused by anything? It
-        // should probably be deleted, but that's outside of this pass' scope.
-        if (RegFakeUses.contains(Reg)) {
+        // There should typically be an exact match between the loaded register
+        // and the FAKE_USE, but sometimes regalloc will choose to load a larger
+        // value than is needed. Therefore, as long as the load isn't used by
+        // anything except at least one FAKE_USE, we will delete it. If it isn't
+        // used by any fake uses, it should still be safe to delete but we
+        // choose to ignore it so that this pass has no side effects unrelated
+        // to fake uses.
+        SmallDenseSet<MachineInstr *> FakeUsesToDelete;
+        SmallVector<MachineInstr *> RemainingFakeUses;
+        for (MachineInstr *&FakeUse : reverse(RegFakeUses)) {
+          if (FakeUse->readsRegister(Reg, TRI)) {
+            FakeUsesToDelete.insert(FakeUse);
+            RegFakeUses.erase(&FakeUse);
+          }
+        }
+        if (!FakeUsesToDelete.empty()) {
           LLVM_DEBUG(dbgs() << "RemoveLoadsIntoFakeUses: DELETING: " << MI);
-          // It is possible that some DBG_VALUE instructions refer to this
-          // instruction. They will be deleted in the live debug variable
-          // analysis.
+          // Since this load only exists to restore a spilled register and we
+          // haven't, run LiveDebugValues yet, there shouldn't be any DBG_VALUEs
+          // for this load; otherwise, deleting this would be incorrect.
           MI.eraseFromParent();
           AnyChanges = true;
           ++NumLoadsDeleted;
-          // Each FAKE_USE now appears to be a fake use of the previous value
-          // of the loaded register; delete them to avoid incorrectly
-          // interpreting them as such.
-          for (MachineInstr *FakeUse : RegFakeUses[Reg]) {
+          for (MachineInstr *FakeUse : FakeUsesToDelete) {
             LLVM_DEBUG(dbgs()
                        << "RemoveLoadsIntoFakeUses: DELETING: " << *FakeUse);
             FakeUse->eraseFromParent();
           }
-          NumFakeUsesDeleted += RegFakeUses[Reg].size();
-          RegFakeUses[Reg].clear();
+          NumFakeUsesDeleted += FakeUsesToDelete.size();
         }
         continue;
       }
@@ -143,13 +155,15 @@ bool RemoveLoadsIntoFakeUses::runOnMachineFunction(MachineFunction &MF) {
       // that register.
       if (!RegFakeUses.empty()) {
         for (const MachineOperand &MO : MI.operands()) {
-          if (MO.isReg() && MO.isDef()) {
-            Register Reg = MO.getReg();
-            assert(Reg.isPhysical() &&
-                   "VReg seen in function with NoVRegs set?");
-            for (MCRegUnit Unit : TRI->regunits(Reg))
-              RegFakeUses.erase(Unit);
-          }
+          if (!MO.isReg())
+            continue;
+          Register Reg = MO.getReg();
+          // We clear RegFakeUses for this register and all subregisters,
+          // because any such FAKE_USE encountered prior is no longer relevant
+          // for later encountered loads.
+          for (MachineInstr *&FakeUse : reverse(RegFakeUses))
+            if (FakeUse->readsRegister(Reg, TRI))
+              RegFakeUses.erase(&FakeUse);
         }
       }
       LivePhysRegs.stepBackward(MI);
diff --git a/llvm/test/CodeGen/X86/fake-use-remove-loads.mir b/llvm/test/CodeGen/X86/fake-use-remove-loads.mir
@@ -0,0 +1,171 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 5
+# Ensure that loads into FAKE_USEs are correctly removed by the
+# remove-loads-into-fake-uses pass, and that if the function does not use
+# instruction referencing then no changes are made.
+# RUN: llc %s -run-pass remove-loads-into-fake-uses -mtriple=x86_64-unknown-linux -debug-only=remove-loads-into-fake-uses 2>&1 -o - | FileCheck %s
+# REQUIRES: asserts
+#
+## We verify that:
+##   - The load into the FAKE_USE is removed, along with the FAKE_USE itself,
+##     even when the FAKE_USE is for a subregister of the move.
+##   - We correctly handle situations where FAKE_USE has additional `killed`
+##     operands added by other passes.
+##   - The store to the stack slot still exists.
+##   - When the register has a use between the restore and the FAKE_USE, we do
+##     not delete the load or fake use.
+
+
+---
+name:            enabled
+alignment:       16
+tracksRegLiveness: true
+noPhis:          true
+noVRegs:         true
+hasFakeUses:     true
+tracksDebugUserValues: true
+debugInstrRef: true
+liveins:
+  - { reg: '$rdi', virtual-reg: '' }
+  - { reg: '$esi', virtual-reg: '' }
+  - { reg: '$rdx', virtual-reg: '' }
+frameInfo:
+  isCalleeSavedInfoValid: true
+stack:
+  - { id: 0, name: '', type: spill-slot, offset: -8, size: 8, alignment: 8,
+      stack-id: default, callee-saved-register: '', callee-saved-restored: true,
+      debug-info-variable: '', debug-info-expression: '', debug-info-location: '' }
+  - { id: 1, name: '', type: spill-slot, offset: -16, size: 8, alignment: 8,
+      stack-id: default, callee-saved-register: '', callee-saved-restored: true,
+      debug-info-variable: '', debug-info-expression: '', debug-info-location: '' }
+body:             |
+  bb.0:
+    liveins: $esi, $rdi, $rdx, $r15, $r14, $r13, $r12, $r11, $rbx
+
+    ; CHECK-LABEL: name: enabled
+    ; CHECK: liveins: $esi, $rdi, $rdx, $r15, $r14, $r13, $r12, $r11, $rbx
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: $rbx = MOV64rr $rdx
+    ; CHECK-NEXT: $r14d = MOV32rr $esi
+    ; CHECK-NEXT: $r15 = MOV64rr $rdi
+    ; CHECK-NEXT: renamable $r12d = XOR32rr undef $r12d, undef $r12d, implicit-def dead $eflags, implicit-def $r12
+    ; CHECK-NEXT: renamable $eax = XOR32rr undef $eax, undef $eax, implicit-def dead $eflags, implicit-def $rax
+
+    ;; The store to the stack slot is still present.
+    ; CHECK-NEXT: MOV64mr $rbp, 1, $noreg, -48, $noreg, killed renamable $rax :: (store (s64) into %stack.0)
+
+    ; CHECK-NEXT: MOV64mr $rbp, 1, $noreg, -40, $noreg, killed renamable $r11 :: (store (s64) into %stack.1)
+    ; CHECK-NEXT: renamable $eax = XOR32rr undef $eax, undef $eax, implicit-def dead $eflags
+    ; CHECK-NEXT: $r13d = MOV32rr killed $eax
+    ; CHECK-NEXT: $rdi = MOV64rr $r15
+    ; CHECK-NEXT: CALL64r renamable $r12, csr_64, implicit $rsp, implicit $ssp, implicit $rdi, implicit-def $rsp, implicit-def $ssp
+    ; CHECK-NEXT: dead renamable $eax = MOV32rm renamable $rbx, 1, $noreg, 0, $noreg
+    ; CHECK-NEXT: renamable $eax = MOV32ri 1
+    ; CHECK-NEXT: TEST8ri renamable $r14b, 1, implicit-def $eflags
+    
+    ;; First FAKE_USE and its corresponding load are removed; second FAKE_USE of
+    ;; a restored value that is also used is preserved.
+    ; CHECK-NEXT: renamable $r11 = MOV64rm $rbp, 1, $noreg, -40, $noreg :: (load (s64) from %stack.1)
+    ; CHECK-NEXT: renamable $r12d = XOR32rr $r12d, $r11d, implicit-def dead $eflags
+    ; CHECK-NEXT: FAKE_USE killed renamable $r11d
+
+    ; CHECK-NEXT: TEST32rr killed renamable $r13d, renamable $r13d, implicit-def $eflags
+    ; CHECK-NEXT: RET64
+
+    $rbx = MOV64rr $rdx
+    $r14d = MOV32rr $esi
+    $r15 = MOV64rr $rdi
+    renamable $r12d = XOR32rr undef $r12d, undef $r12d, implicit-def dead $eflags, implicit-def $r12
+    renamable $eax = XOR32rr undef $eax, undef $eax, implicit-def dead $eflags, implicit-def $rax
+    MOV64mr $rbp, 1, $noreg, -48, $noreg, killed renamable $rax :: (store (s64) into %stack.0)
+    MOV64mr $rbp, 1, $noreg, -40, $noreg, killed renamable $r11 :: (store (s64) into %stack.1)
+    renamable $eax = XOR32rr undef $eax, undef $eax, implicit-def dead $eflags
+    $r13d = MOV32rr killed $eax
+    $rdi = MOV64rr $r15
+    CALL64r renamable $r12, csr_64, implicit $rsp, implicit $ssp, implicit $rdi, implicit-def $rsp, implicit-def $ssp
+    dead renamable $eax = MOV32rm renamable $rbx, 1, $noreg, 0, $noreg
+    renamable $eax = MOV32ri 1
+    TEST8ri renamable $r14b, 1, implicit-def $eflags
+    renamable $rax = MOV64rm $rbp, 1, $noreg, -48, $noreg :: (load (s64) from %stack.0)
+    FAKE_USE renamable $eax, implicit killed $rax
+    renamable $r11 = MOV64rm $rbp, 1, $noreg, -40, $noreg :: (load (s64) from %stack.1)
+    renamable $r12d = XOR32rr $r12d, $r11d, implicit-def dead $eflags
+    FAKE_USE killed renamable $r11d
+    TEST32rr killed renamable $r13d, renamable $r13d, implicit-def $eflags
+    RET64
+
+...
+---
+name:            disabled
+alignment:       16
+tracksRegLiveness: true
+noPhis:          true
+noVRegs:         true
+hasFakeUses:     true
+tracksDebugUserValues: true
+debugInstrRef: false
+liveins:
+  - { reg: '$rdi', virtual-reg: '' }
+  - { reg: '$esi', virtual-reg: '' }
+  - { reg: '$rdx', virtual-reg: '' }
+frameInfo:
+  isCalleeSavedInfoValid: true
+stack:
+  - { id: 0, name: '', type: spill-slot, offset: -8, size: 8, alignment: 8,
+      stack-id: default, callee-saved-register: '', callee-saved-restored: true,
+      debug-info-variable: '', debug-info-expression: '', debug-info-location: '' }
+  - { id: 1, name: '', type: spill-slot, offset: -16, size: 8, alignment: 8,
+      stack-id: default, callee-saved-register: '', callee-saved-restored: true,
+      debug-info-variable: '', debug-info-expression: '', debug-info-location: '' }
+body:             |
+  bb.0:
+    liveins: $esi, $rdi, $rdx, $r15, $r14, $r13, $r12, $r11, $rbx
+
+    ; CHECK-LABEL: name: disabled
+    ; CHECK: liveins: $esi, $rdi, $rdx, $r15, $r14, $r13, $r12, $r11, $rbx
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: $rbx = MOV64rr $rdx
+    ; CHECK-NEXT: $r14d = MOV32rr $esi
+    ; CHECK-NEXT: $r15 = MOV64rr $rdi
+    ; CHECK-NEXT: renamable $r12d = XOR32rr undef $r12d, undef $r12d, implicit-def dead $eflags, implicit-def $r12
+    ; CHECK-NEXT: renamable $eax = XOR32rr undef $eax, undef $eax, implicit-def dead $eflags, implicit-def $rax
+    ; CHECK-NEXT: MOV64mr $rbp, 1, $noreg, -48, $noreg, killed renamable $rax :: (store (s64) into %stack.0)
+    ; CHECK-NEXT: MOV64mr $rbp, 1, $noreg, -40, $noreg, killed renamable $r11 :: (store (s64) into %stack.1)
+    ; CHECK-NEXT: renamable $eax = XOR32rr undef $eax, undef $eax, implicit-def dead $eflags
+    ; CHECK-NEXT: $r13d = MOV32rr killed $eax
+    ; CHECK-NEXT: $rdi = MOV64rr $r15
+    ; CHECK-NEXT: CALL64r renamable $r12, csr_64, implicit $rsp, implicit $ssp, implicit $rdi, implicit-def $rsp, implicit-def $ssp
+    ; CHECK-NEXT: dead renamable $eax = MOV32rm renamable $rbx, 1, $noreg, 0, $noreg
+    ; CHECK-NEXT: renamable $eax = MOV32ri 1
+    ; CHECK-NEXT: TEST8ri renamable $r14b, 1, implicit-def $eflags
+
+    ;; Verify that when instr-ref is disabled, we do not remove fake uses.
+    ; CHECK-NEXT: renamable $rax = MOV64rm $rbp, 1, $noreg, -48, $noreg :: (load (s64) from %stack.0)
+    ; CHECK-NEXT: FAKE_USE renamable $eax, implicit killed $rax
+    ; CHECK-NEXT: renamable $r11 = MOV64rm $rbp, 1, $noreg, -40, $noreg :: (load (s64) from %stack.1)
+    ; CHECK-NEXT: renamable $r12d = XOR32rr $r12d, $r11d, implicit-def dead $eflags
+    ; CHECK-NEXT: FAKE_USE killed renamable $r11d
+    ; CHECK-NEXT: TEST32rr killed renamable $r13d, renamable $r13d, implicit-def $eflags
+    ; CHECK-NEXT: RET64
+    $rbx = MOV64rr $rdx
+    $r14d = MOV32rr $esi
+    $r15 = MOV64rr $rdi
+    renamable $r12d = XOR32rr undef $r12d, undef $r12d, implicit-def dead $eflags, implicit-def $r12
+    renamable $eax = XOR32rr undef $eax, undef $eax, implicit-def dead $eflags, implicit-def $rax
+    MOV64mr $rbp, 1, $noreg, -48, $noreg, killed renamable $rax :: (store (s64) into %stack.0)
+    MOV64mr $rbp, 1, $noreg, -40, $noreg, killed renamable $r11 :: (store (s64) into %stack.1)
+    renamable $eax = XOR32rr undef $eax, undef $eax, implicit-def dead $eflags
+    $r13d = MOV32rr killed $eax
+    $rdi = MOV64rr $r15
+    CALL64r renamable $r12, csr_64, implicit $rsp, implicit $ssp, implicit $rdi, implicit-def $rsp, implicit-def $ssp
+    dead renamable $eax = MOV32rm renamable $rbx, 1, $noreg, 0, $noreg
+    renamable $eax = MOV32ri 1
+    TEST8ri renamable $r14b, 1, implicit-def $eflags
+    renamable $rax = MOV64rm $rbp, 1, $noreg, -48, $noreg :: (load (s64) from %stack.0)
+    FAKE_USE renamable $eax, implicit killed $rax
+    renamable $r11 = MOV64rm $rbp, 1, $noreg, -40, $noreg :: (load (s64) from %stack.1)
+    renamable $r12d = XOR32rr $r12d, $r11d, implicit-def dead $eflags
+    FAKE_USE killed renamable $r11d
+    TEST32rr killed renamable $r13d, renamable $r13d, implicit-def $eflags
+    RET64
+
+...
