[ARM] Replace TransferImpOps with copyImplicitOps

In most places where TransferImpOps is currently used we just have one machine instruction, so it's doing the same thing as copyImplicitOps anyway. In those cases where we have more than one machine instruction the destination is written to in each instruction so any implicit defs should appear on all of them (and we shouldn't see any implicit refs as these pseudo-instruction don't have any register inputs), meaning the current use of TransferImpOps is incorrect and we should be using copyImplicitOps on all of the generated instructions. Differential Revision: https://reviews.llvm.org/D155301
llvm · Jul 18, 2023 · 343e204 · 343e204
1 parent 46aec7b
commit 343e204
Show file tree

Hide file tree

Showing 3 changed files with 146 additions and 77 deletions.
diff --git a/llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp b/llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp
@@ -60,8 +60,6 @@ namespace {
     }
 
   private:
-    void TransferImpOps(MachineInstr &OldMI,
-                        MachineInstrBuilder &UseMI, MachineInstrBuilder &DefMI);
     bool ExpandMI(MachineBasicBlock &MBB,
                   MachineBasicBlock::iterator MBBI,
                   MachineBasicBlock::iterator &NextMBBI);
@@ -123,22 +121,6 @@ namespace {
 INITIALIZE_PASS(ARMExpandPseudo, DEBUG_TYPE, ARM_EXPAND_PSEUDO_NAME, false,
                 false)
 
-/// TransferImpOps - Transfer implicit operands on the pseudo instruction to
-/// the instructions created from the expansion.
-void ARMExpandPseudo::TransferImpOps(MachineInstr &OldMI,
-                                     MachineInstrBuilder &UseMI,
-                                     MachineInstrBuilder &DefMI) {
-  const MCInstrDesc &Desc = OldMI.getDesc();
-  for (const MachineOperand &MO :
-       llvm::drop_begin(OldMI.operands(), Desc.getNumOperands())) {
-    assert(MO.isReg() && MO.getReg());
-    if (MO.isUse())
-      UseMI.add(MO);
-    else
-      DefMI.add(MO);
-  }
-}
-
 namespace {
   // Constants for register spacing in NEON load/store instructions.
   // For quad-register load-lane and store-lane pseudo instructors, the
@@ -678,7 +660,7 @@ void ARMExpandPseudo::ExpandVLD(MachineBasicBlock::iterator &MBBI) {
   }
   // Add an implicit def for the super-register.
   MIB.addReg(DstReg, RegState::ImplicitDefine | getDeadRegState(DstIsDead));
-  TransferImpOps(MI, MIB, MIB);
+  MIB.copyImplicitOps(MI);
 
   // Transfer memoperands.
   MIB.cloneMemRefs(MI);
@@ -754,7 +736,7 @@ void ARMExpandPseudo::ExpandVST(MachineBasicBlock::iterator &MBBI) {
     MIB->addRegisterKilled(SrcReg, TRI, true);
   else if (!SrcIsUndef)
     MIB.addReg(SrcReg, RegState::Implicit); // Add implicit uses for src reg.
-  TransferImpOps(MI, MIB, MIB);
+  MIB.copyImplicitOps(MI);
 
   // Transfer memoperands.
   MIB.cloneMemRefs(MI);
@@ -846,7 +828,7 @@ void ARMExpandPseudo::ExpandLaneOp(MachineBasicBlock::iterator &MBBI) {
   if (TableEntry->IsLoad)
     // Add an implicit def for the super-register.
     MIB.addReg(DstReg, RegState::ImplicitDefine | getDeadRegState(DstIsDead));
-  TransferImpOps(MI, MIB, MIB);
+  MIB.copyImplicitOps(MI);
   // Transfer memoperands.
   MIB.cloneMemRefs(MI);
   MI.eraseFromParent();
@@ -886,7 +868,7 @@ void ARMExpandPseudo::ExpandVTBL(MachineBasicBlock::iterator &MBBI,
 
   // Add an implicit kill and use for the super-reg.
   MIB.addReg(SrcReg, RegState::Implicit | getKillRegState(SrcIsKill));
-  TransferImpOps(MI, MIB, MIB);
+  MIB.copyImplicitOps(MI);
   MI.eraseFromParent();
   LLVM_DEBUG(dbgs() << "To:        "; MIB.getInstr()->dump(););
 }
@@ -923,7 +905,7 @@ void ARMExpandPseudo::ExpandMQQPRLoadStore(MachineBasicBlock::iterator &MBBI) {
   if (NewOpc == ARM::VSTMDIA)
     MIB.addReg(SrcReg, RegState::Implicit);
 
-  TransferImpOps(MI, MIB, MIB);
+  MIB.copyImplicitOps(MI);
   MIB.cloneMemRefs(MI);
   MI.eraseFromParent();
 }
@@ -1132,7 +1114,8 @@ void ARMExpandPseudo::ExpandMOV32BitImm(MachineBasicBlock &MBB,
     HI16.addImm(Pred).addReg(PredReg).add(condCodeOp());
     if (isCC)
       LO16.add(makeImplicit(MI.getOperand(1)));
-    TransferImpOps(MI, LO16, HI16);
+    LO16.copyImplicitOps(MI);
+    HI16.copyImplicitOps(MI);
     MI.eraseFromParent();
     return;
   }
@@ -1191,7 +1174,8 @@ void ARMExpandPseudo::ExpandMOV32BitImm(MachineBasicBlock &MBB,
 
   if (isCC)
     LO16.add(makeImplicit(MI.getOperand(1)));
-  TransferImpOps(MI, LO16, HI16);
+  LO16.copyImplicitOps(MI);
+  HI16.copyImplicitOps(MI);
   MI.eraseFromParent();
   LLVM_DEBUG(dbgs() << "To:        "; LO16.getInstr()->dump(););
   LLVM_DEBUG(dbgs() << "And:       "; HI16.getInstr()->dump(););
@@ -2584,14 +2568,13 @@ bool ARMExpandPseudo::ExpandMI(MachineBasicBlock &MBB,
     }
     case ARM::RRX: {
       // This encodes as "MOVs Rd, Rm, rrx
-      MachineInstrBuilder MIB =
-          BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(ARM::MOVsi),
-                  MI.getOperand(0).getReg())
-              .add(MI.getOperand(1))
-              .addImm(ARM_AM::getSORegOpc(ARM_AM::rrx, 0))
-              .add(predOps(ARMCC::AL))
-              .add(condCodeOp());
-      TransferImpOps(MI, MIB, MIB);
+      BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(ARM::MOVsi),
+              MI.getOperand(0).getReg())
+          .add(MI.getOperand(1))
+          .addImm(ARM_AM::getSORegOpc(ARM_AM::rrx, 0))
+          .add(predOps(ARMCC::AL))
+          .add(condCodeOp())
+          .copyImplicitOps(MI);
       MI.eraseFromParent();
       return true;
     }
@@ -2631,7 +2614,7 @@ bool ARMExpandPseudo::ExpandMI(MachineBasicBlock &MBB,
       }
 
       MIB.cloneMemRefs(MI);
-      TransferImpOps(MI, MIB, MIB);
+      MIB.copyImplicitOps(MI);
       // Update the call site info.
       if (MI.isCandidateForCallSiteEntry())
         MF->moveCallSiteInfo(&MI, &*MIB);
@@ -2644,17 +2627,16 @@ bool ARMExpandPseudo::ExpandMI(MachineBasicBlock &MBB,
         ? ARM::tLDRpci : ARM::t2LDRpci;
       Register DstReg = MI.getOperand(0).getReg();
       bool DstIsDead = MI.getOperand(0).isDead();
-      MachineInstrBuilder MIB1 =
-          BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(NewLdOpc), DstReg)
-              .add(MI.getOperand(1))
-              .add(predOps(ARMCC::AL));
-      MIB1.cloneMemRefs(MI);
-      MachineInstrBuilder MIB2 =
-          BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(ARM::tPICADD))
-              .addReg(DstReg, RegState::Define | getDeadRegState(DstIsDead))
-              .addReg(DstReg)
-              .add(MI.getOperand(2));
-      TransferImpOps(MI, MIB1, MIB2);
+      BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(NewLdOpc), DstReg)
+          .add(MI.getOperand(1))
+          .add(predOps(ARMCC::AL))
+          .cloneMemRefs(MI)
+          .copyImplicitOps(MI);
+      BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(ARM::tPICADD))
+          .addReg(DstReg, RegState::Define | getDeadRegState(DstIsDead))
+          .addReg(DstReg)
+          .add(MI.getOperand(2))
+          .copyImplicitOps(MI);
       MI.eraseFromParent();
       return true;
     }
@@ -2739,15 +2721,16 @@ bool ARMExpandPseudo::ExpandMI(MachineBasicBlock &MBB,
       unsigned PICAddOpc = isARM
         ? (Opcode == ARM::MOV_ga_pcrel_ldr ? ARM::PICLDR : ARM::PICADD)
         : ARM::tPICADD;
-      MachineInstrBuilder MIB1 = BuildMI(MBB, MBBI, MI.getDebugLoc(),
-                                         TII->get(LO16Opc), DstReg)
-        .addGlobalAddress(GV, MO1.getOffset(), TF | LO16TF)
-        .addImm(LabelId);
+      BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(LO16Opc), DstReg)
+          .addGlobalAddress(GV, MO1.getOffset(), TF | LO16TF)
+          .addImm(LabelId)
+          .copyImplicitOps(MI);
 
       BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(HI16Opc), DstReg)
-        .addReg(DstReg)
-        .addGlobalAddress(GV, MO1.getOffset(), TF | HI16TF)
-        .addImm(LabelId);
+          .addReg(DstReg)
+          .addGlobalAddress(GV, MO1.getOffset(), TF | HI16TF)
+          .addImm(LabelId)
+          .copyImplicitOps(MI);
 
       MachineInstrBuilder MIB3 = BuildMI(MBB, MBBI, MI.getDebugLoc(),
                                          TII->get(PICAddOpc))
@@ -2758,7 +2741,7 @@ bool ARMExpandPseudo::ExpandMI(MachineBasicBlock &MBB,
         if (Opcode == ARM::MOV_ga_pcrel_ldr)
           MIB3.cloneMemRefs(MI);
       }
-      TransferImpOps(MI, MIB1, MIB3);
+      MIB3.copyImplicitOps(MI);
       MI.eraseFromParent();
       return true;
     }
@@ -2786,14 +2769,13 @@ bool ARMExpandPseudo::ExpandMI(MachineBasicBlock &MBB,
       return true;
 
     case ARM::SUBS_PC_LR: {
-      MachineInstrBuilder MIB =
-          BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(ARM::SUBri), ARM::PC)
-              .addReg(ARM::LR)
-              .add(MI.getOperand(0))
-              .add(MI.getOperand(1))
-              .add(MI.getOperand(2))
-              .addReg(ARM::CPSR, RegState::Undef);
-      TransferImpOps(MI, MIB, MIB);
+      BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(ARM::SUBri), ARM::PC)
+          .addReg(ARM::LR)
+          .add(MI.getOperand(0))
+          .add(MI.getOperand(1))
+          .add(MI.getOperand(2))
+          .addReg(ARM::CPSR, RegState::Undef)
+          .copyImplicitOps(MI);
       MI.eraseFromParent();
       return true;
     }
@@ -2822,7 +2804,7 @@ bool ARMExpandPseudo::ExpandMI(MachineBasicBlock &MBB,
 
       // Add an implicit def for the super-register.
       MIB.addReg(DstReg, RegState::ImplicitDefine | getDeadRegState(DstIsDead));
-      TransferImpOps(MI, MIB, MIB);
+      MIB.copyImplicitOps(MI);
       MIB.cloneMemRefs(MI);
       MI.eraseFromParent();
       return true;
@@ -2855,7 +2837,7 @@ bool ARMExpandPseudo::ExpandMI(MachineBasicBlock &MBB,
       if (SrcIsKill)      // Add an implicit kill for the Q register.
         MIB->addRegisterKilled(SrcReg, TRI, true);
 
-      TransferImpOps(MI, MIB, MIB);
+      MIB.copyImplicitOps(MI);
       MIB.cloneMemRefs(MI);
       MI.eraseFromParent();
       return true;

diff --git a/llvm/test/CodeGen/ARM/expand-pseudos.mir b/llvm/test/CodeGen/ARM/expand-pseudos.mir
@@ -15,6 +15,15 @@
   entry:
     unreachable
   }
+  define i32 @test4(i32 %x) {
+  entry:
+    unreachable
+  }
+  @var = global i32 0
+  define i32 @test5(i32 %x) {
+  entry:
+    unreachable
+  }
 ...
 ---
 name:            test1
@@ -28,11 +37,12 @@ body:             |
 
     ; CHECK-LABEL: name: test1
     ; CHECK: liveins: $r0
-    ; CHECK: $r1 = MOVi 2, 14 /* CC::al */, $noreg, $noreg
-    ; CHECK: CMPri killed $r0, 0, 14 /* CC::al */, $noreg, implicit-def $cpsr
-    ; CHECK: $r1 = MOVi16 500, 0 /* CC::eq */, killed $cpsr, implicit killed $r1
-    ; CHECK: $r0 = MOVr killed $r1, 14 /* CC::al */, $noreg, $noreg
-    ; CHECK: BX_RET 14 /* CC::al */, $noreg, implicit $r0
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: $r1 = MOVi 2, 14 /* CC::al */, $noreg, $noreg
+    ; CHECK-NEXT: CMPri killed $r0, 0, 14 /* CC::al */, $noreg, implicit-def $cpsr
+    ; CHECK-NEXT: $r1 = MOVi16 500, 0 /* CC::eq */, killed $cpsr, implicit killed $r1
+    ; CHECK-NEXT: $r0 = MOVr killed $r1, 14 /* CC::al */, $noreg, $noreg
+    ; CHECK-NEXT: BX_RET 14 /* CC::al */, $noreg, implicit $r0
     $r1 = MOVi 2, 14, $noreg, $noreg
     CMPri killed $r0, 0, 14, $noreg, implicit-def $cpsr
     $r1 = MOVCCi16 killed $r1, 500, 0, killed $cpsr
@@ -52,12 +62,13 @@ body:             |
 
     ; CHECK-LABEL: name: test2
     ; CHECK: liveins: $r0
-    ; CHECK: $r1 = MOVi 2, 14 /* CC::al */, $noreg, $noreg
-    ; CHECK: CMPri killed $r0, 0, 14 /* CC::al */, $noreg, implicit-def $cpsr
-    ; CHECK: $r1 = MOVi16 2068, 0 /* CC::eq */, $cpsr, implicit killed $r1
-    ; CHECK: $r1 = MOVTi16 $r1, 7637, 0 /* CC::eq */, $cpsr
-    ; CHECK: $r0 = MOVr killed $r1, 14 /* CC::al */, $noreg, $noreg
-    ; CHECK: BX_RET 14 /* CC::al */, $noreg, implicit $r0
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: $r1 = MOVi 2, 14 /* CC::al */, $noreg, $noreg
+    ; CHECK-NEXT: CMPri killed $r0, 0, 14 /* CC::al */, $noreg, implicit-def $cpsr
+    ; CHECK-NEXT: $r1 = MOVi16 2068, 0 /* CC::eq */, $cpsr, implicit killed $r1
+    ; CHECK-NEXT: $r1 = MOVTi16 $r1, 7637, 0 /* CC::eq */, $cpsr
+    ; CHECK-NEXT: $r0 = MOVr killed $r1, 14 /* CC::al */, $noreg, $noreg
+    ; CHECK-NEXT: BX_RET 14 /* CC::al */, $noreg, implicit $r0
     $r1 = MOVi 2, 14, $noreg, $noreg
     CMPri killed $r0, 0, 14, $noreg, implicit-def $cpsr
     $r1 = MOVCCi32imm killed $r1, 500500500, 0, killed $cpsr
@@ -78,11 +89,55 @@ body:             |
 
     ; CHECK-LABEL: name: test3
     ; CHECK: liveins: $r0, $r1
-    ; CHECK: CMPri $r1, 500, 14 /* CC::al */, $noreg, implicit-def $cpsr
-    ; CHECK: $r0 = MOVr killed $r1, 12 /* CC::gt */, killed $cpsr, $noreg, implicit killed $r0
-    ; CHECK: BX_RET 14 /* CC::al */, $noreg, implicit $r0
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: CMPri $r1, 500, 14 /* CC::al */, $noreg, implicit-def $cpsr
+    ; CHECK-NEXT: $r0 = MOVr killed $r1, 12 /* CC::gt */, killed $cpsr, $noreg, implicit killed $r0
+    ; CHECK-NEXT: BX_RET 14 /* CC::al */, $noreg, implicit $r0
     CMPri $r1, 500, 14, $noreg, implicit-def $cpsr
     $r0 = MOVCCr killed $r0, killed $r1, 12, killed $cpsr
     BX_RET 14, $noreg, implicit $r0
 
 ...
+---
+name:            test4
+alignment:       4
+tracksRegLiveness: true
+liveins:
+  - { reg: '$r0', virtual-reg: '' }
+  - { reg: '$r0_r1', virtual-reg: '' }
+body:             |
+  bb.0.entry:
+    liveins: $r0, $r0_r1
+
+    ; CHECK-LABEL: name: test4
+    ; CHECK: liveins: $r0, $r0_r1
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: $r0 = MOVi16 51712, 14 /* CC::al */, $noreg, implicit-def $r0_r1
+    ; CHECK-NEXT: $r0 = MOVTi16 $r0, 15258, 14 /* CC::al */, $noreg, implicit-def $r0_r1
+    ; CHECK-NEXT: BX_RET 14 /* CC::al */, $noreg, implicit $r0
+    $r0 = MOVi32imm 1000000000, implicit-def $r0_r1
+    BX_RET 14, $noreg, implicit $r0
+
+...
+---
+name:            test5
+alignment:       4
+tracksRegLiveness: true
+liveins:
+  - { reg: '$r0', virtual-reg: '' }
+  - { reg: '$r0_r1', virtual-reg: '' }
+body:             |
+  bb.0.entry:
+    liveins: $r0, $r0_r1
+
+    ; CHECK-LABEL: name: test5
+    ; CHECK: liveins: $r0, $r0_r1
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: $r0 = MOVi16_ga_pcrel target-flags(arm-lo16) @var, 0, implicit-def $r0_r1
+    ; CHECK-NEXT: $r0 = MOVTi16_ga_pcrel $r0, target-flags(arm-hi16) @var, 0, implicit-def $r0_r1
+    ; CHECK-NEXT: $r0 = PICLDR $r0, 0, 14 /* CC::al */, $noreg, implicit-def $r0_r1
+    ; CHECK-NEXT: BX_RET 14 /* CC::al */, $noreg, implicit $r0
+    $r0 = MOV_ga_pcrel_ldr @var, implicit-def $r0_r1
+    BX_RET 14, $noreg, implicit $r0
+
+...
diff --git a/llvm/test/CodeGen/Thumb2/expand-pseudos.mir b/llvm/test/CodeGen/Thumb2/expand-pseudos.mir
@@ -0,0 +1,32 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -run-pass=arm-pseudo -verify-machineinstrs %s -o - | FileCheck %s
+--- |
+  target triple = "thumbv7---gnueabi"
+
+  @var = global i32 0
+  define i32 @test1(i32 %x) {
+  entry:
+    unreachable
+  }
+...
+---
+name:            test1
+alignment:       4
+tracksRegLiveness: true
+liveins:
+  - { reg: '$r0', virtual-reg: '' }
+  - { reg: '$r0_r1', virtual-reg: '' }
+body:             |
+  bb.0.entry:
+    liveins: $r0, $r0_r1
+
+    ; CHECK-LABEL: name: test1
+    ; CHECK: liveins: $r0, $r0_r1
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: $r0 = t2LDRpci @var, 14 /* CC::al */, $noreg, implicit-def $r0_r1
+    ; CHECK-NEXT: $r0 = tPICADD $r0, 0, implicit-def $r0_r1
+    ; CHECK-NEXT: BX_RET 14 /* CC::al */, $noreg, implicit $r0
+    $r0 = t2LDRpci_pic @var, 0, implicit-def $r0_r1
+    BX_RET 14, $noreg, implicit $r0
+
+...