[ARM] Make ARMExpandPseudo add implicit uses for predicated instructions

[efriedma]

Summary

Missing these could potentially screw up post-ra scheduling.

Issue found by inspection, so I don't have a real testcase. Included test just verifies the expected operands after expansion.

[rovka]

Hi Eli,

I couldn't find an awful lot of tests for ARM pseudo expansion in particular, so I think it would be nice to add one. A MIR test checking each of the transformations that you touched in this patch would work fine.

I've been looking into this area myself recently and found it all a bit confusing. I have a few questions inline, sorry if they're a bit basic.

Thanks.

lib/Target/ARM/ARMExpandPseudoInsts.cpp

1086

How exactly does this work? You're already passing the register from MI.getOperand(1) to BuildMI, which IIUC will mark it as a define. Why do you also need to make it implicit?

1140

IIUC, MOVi16 is AI1, not AsI1. Doesn't that mean it shouldn't set any CC, so you don't need an implicit operand for it at all?

[efriedma]

OK, I'll add a testcase.

lib/Target/ARM/ARMExpandPseudoInsts.cpp

1086

We also need to mark it as a use (the instruction is predicated, so if the predicate is false, the value isn't modified).

It's an implicit use because it isn't encoded anywhere in the instruction.

1140

It doesn't modify CPSR. It uses CPSR ("MI.getOperand(4)"). I'm not sure how that's relevant to this patch, though.

[efriedma]

Added testcases (didn't add a testcase for every single opcode, since there's a lot of them, but this should be enough to illustrate what's happening).

[rovka]

Thanks for the test-case and the explanations.
LGTM but you should probably wait for someone more versed in these things to give the final approval.

lib/Target/ARM/ARMExpandPseudoInsts.cpp

1140

Sorry, I don't know where I was going with that, the code looks perfectly fine.

test/CodeGen/ARM/expand-pseudos.mir

69

If you want to move the checks closer to the code, you can use ; CHECK (instead of # CHECK).

[efriedma]

Ping

[efriedma]

Ping^2

[efriedma]

Ping^3.

[olista01]

This LGTM, but could you please commit the pass registration changes as a separate commit, as they are not really related to this change.

lib/Target/ARM/ARM.h

 
 void computeBlockSize(MachineFunction *MF, MachineBasicBlock *MBB,
                       BasicBlockInfo &BBI);
 std::vector<BasicBlockInfo> computeAllBlockSizes(MachineFunction *MF);
 
 void initializeARMLoadStoreOptPass(PassRegistry &);
 void initializeARMPreAllocLoadStoreOptPass(PassRegistry &);
 void initializeARMConstantIslandsPass(PassRegistry &);
+void initializeARMExpandPseudoPass(PassRegistry &);
 
 } // end namespace llvm
 
 #endif // LLVM_LIB_TARGET_ARM_ARM_H

lib/Target/ARM/ARMExpandPseudoInsts.cpp

 using namespace llvm;
 
 #define DEBUG_TYPE "arm-pseudo"
 
 static cl::opt<bool>
 VerifyARMPseudo("verify-arm-pseudo-expand", cl::Hidden,
                 cl::desc("Verify machine code after expanding ARM pseudos"));
 
+#define ARM_EXPAND_PSEUDO_NAME "ARM pseudo instruction expansion pass"
+
 namespace {
   class ARMExpandPseudo : public MachineFunctionPass {
   public:
     static char ID;
     ARMExpandPseudo() : MachineFunctionPass(ID) {}
 
     const ARMBaseInstrInfo *TII;
     const TargetRegisterInfo *TRI;
     const ARMSubtarget *STI;
     ARMFunctionInfo *AFI;
 
     bool runOnMachineFunction(MachineFunction &Fn) override;
 
     MachineFunctionProperties getRequiredProperties() const override {
       return MachineFunctionProperties().set(
           MachineFunctionProperties::Property::NoVRegs);
     }
 
     StringRef getPassName() const override {
-      return "ARM pseudo instruction expansion pass";
+      return ARM_EXPAND_PSEUDO_NAME;
     }
 
   private:
     void TransferImpOps(MachineInstr &OldMI,
                         MachineInstrBuilder &UseMI, MachineInstrBuilder &DefMI);
     bool ExpandMI(MachineBasicBlock &MBB,
                   MachineBasicBlock::iterator MBBI,
                   MachineBasicBlock::iterator &NextMBBI);
private:
 
     bool ExpandCMP_SWAP_64(MachineBasicBlock &MBB,
                            MachineBasicBlock::iterator MBBI,
                            MachineBasicBlock::iterator &NextMBBI);
   };
   char ARMExpandPseudo::ID = 0;
 }
 
+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 (unsigned i = Desc.getNumOperands(), e = OldMI.getNumOperands();
        i != e; ++i) {
case MachineOperand::MO_CFIIndex:
     return false;
   case MachineOperand::MO_IntrinsicID:
   case MachineOperand::MO_Predicate:
     llvm_unreachable("should not exist post-isel");
   }
   llvm_unreachable("unhandled machine operand type");
 }
 
+static MachineOperand makeImplicit(const MachineOperand &MO) {
+  MachineOperand NewMO = MO;
+  NewMO.setImplicit();
+  return NewMO;
+}
+
 void ARMExpandPseudo::ExpandMOV32BitImm(MachineBasicBlock &MBB,
                                         MachineBasicBlock::iterator &MBBI) {
   MachineInstr &MI = *MBBI;
   unsigned Opcode = MI.getOpcode();
   unsigned PredReg = 0;
   ARMCC::CondCodes Pred = getInstrPredicate(MI, PredReg);
   unsigned DstReg = MI.getOperand(0).getReg();
   bool DstIsDead = MI.getOperand(0).isDead();
if (!STI->hasV6T2Ops() &&
     unsigned SOImmValV1 = ARM_AM::getSOImmTwoPartFirst(ImmVal);
     unsigned SOImmValV2 = ARM_AM::getSOImmTwoPartSecond(ImmVal);
     LO16 = LO16.addImm(SOImmValV1);
     HI16 = HI16.addImm(SOImmValV2);
     LO16->setMemRefs(MI.memoperands_begin(), MI.memoperands_end());
     HI16->setMemRefs(MI.memoperands_begin(), MI.memoperands_end());
     LO16.addImm(Pred).addReg(PredReg).add(condCodeOp());
     HI16.addImm(Pred).addReg(PredReg).add(condCodeOp());
+    if (isCC)
+      LO16.add(makeImplicit(MI.getOperand(1)));
     TransferImpOps(MI, LO16, HI16);
     MI.eraseFromParent();
     return;
   }
 
   unsigned LO16Opc = 0;
   unsigned HI16Opc = 0;
   if (Opcode == ARM::t2MOVi32imm || Opcode == ARM::t2MOVCCi32imm) {
void ARMExpandPseudo::ExpandMOV32BitImm(MachineBasicBlock &MBB,
   LO16->setMemRefs(MI.memoperands_begin(), MI.memoperands_end());
   HI16->setMemRefs(MI.memoperands_begin(), MI.memoperands_end());
   LO16.addImm(Pred).addReg(PredReg);
   HI16.addImm(Pred).addReg(PredReg);
 
   if (RequiresBundling)
     finalizeBundle(MBB, LO16->getIterator(), MBBI->getIterator());
 
+  if (isCC)
+    LO16.add(makeImplicit(MI.getOperand(1)));
   TransferImpOps(MI, LO16, HI16);
   MI.eraseFromParent();
 }
 
 /// Expand a CMP_SWAP pseudo-inst to an ldrex/strex loop as simply as
 /// possible. This only gets used at -O0 so we don't care about efficiency of
 /// the generated code.
 bool ARMExpandPseudo::ExpandCMP_SWAP(MachineBasicBlock &MBB,
switch (Opcode) {
     }
     case ARM::VMOVScc:
     case ARM::VMOVDcc: {
       unsigned newOpc = Opcode == ARM::VMOVScc ? ARM::VMOVS : ARM::VMOVD;
       BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(newOpc),
               MI.getOperand(1).getReg())
           .add(MI.getOperand(2))
           .addImm(MI.getOperand(3).getImm()) // 'pred'
-          .add(MI.getOperand(4));
+          .add(MI.getOperand(4))
+          .add(makeImplicit(MI.getOperand(1)));
 
       MI.eraseFromParent();
       return true;
     }
     case ARM::t2MOVCCr:
     case ARM::MOVCCr: {
       unsigned Opc = AFI->isThumbFunction() ? ARM::t2MOVr : ARM::MOVr;
       BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(Opc),
               MI.getOperand(1).getReg())
           .add(MI.getOperand(2))
           .addImm(MI.getOperand(3).getImm()) // 'pred'
           .add(MI.getOperand(4))
-          .add(condCodeOp()); // 's' bit
+          .add(condCodeOp()) // 's' bit
+          .add(makeImplicit(MI.getOperand(1)));
 
       MI.eraseFromParent();
       return true;
     }
     case ARM::MOVCCsi: {
       BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(ARM::MOVsi),
               (MI.getOperand(1).getReg()))
           .add(MI.getOperand(2))
           .addImm(MI.getOperand(3).getImm())
           .addImm(MI.getOperand(4).getImm()) // 'pred'
           .add(MI.getOperand(5))
-          .add(condCodeOp()); // 's' bit
+          .add(condCodeOp()) // 's' bit
+          .add(makeImplicit(MI.getOperand(1)));
 
       MI.eraseFromParent();
       return true;
     }
     case ARM::MOVCCsr: {
       BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(ARM::MOVsr),
               (MI.getOperand(1).getReg()))
           .add(MI.getOperand(2))
           .add(MI.getOperand(3))
           .addImm(MI.getOperand(4).getImm())
           .addImm(MI.getOperand(5).getImm()) // 'pred'
           .add(MI.getOperand(6))
-          .add(condCodeOp()); // 's' bit
+          .add(condCodeOp()) // 's' bit
+          .add(makeImplicit(MI.getOperand(1)));
 
       MI.eraseFromParent();
       return true;
     }
     case ARM::t2MOVCCi16:
     case ARM::MOVCCi16: {
       unsigned NewOpc = AFI->isThumbFunction() ? ARM::t2MOVi16 : ARM::MOVi16;
       BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(NewOpc),
               MI.getOperand(1).getReg())
           .addImm(MI.getOperand(2).getImm())
           .addImm(MI.getOperand(3).getImm()) // 'pred'
-          .add(MI.getOperand(4));
+          .add(MI.getOperand(4))
+          .add(makeImplicit(MI.getOperand(1)));
       MI.eraseFromParent();
       return true;
     }
     case ARM::t2MOVCCi:
     case ARM::MOVCCi: {
       unsigned Opc = AFI->isThumbFunction() ? ARM::t2MOVi : ARM::MOVi;
       BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(Opc),
               MI.getOperand(1).getReg())
           .addImm(MI.getOperand(2).getImm())
           .addImm(MI.getOperand(3).getImm()) // 'pred'
           .add(MI.getOperand(4))
-          .add(condCodeOp()); // 's' bit
+          .add(condCodeOp()) // 's' bit
+          .add(makeImplicit(MI.getOperand(1)));
 
       MI.eraseFromParent();
       return true;
     }
     case ARM::t2MVNCCi:
     case ARM::MVNCCi: {
       unsigned Opc = AFI->isThumbFunction() ? ARM::t2MVNi : ARM::MVNi;
       BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(Opc),
               MI.getOperand(1).getReg())
           .addImm(MI.getOperand(2).getImm())
           .addImm(MI.getOperand(3).getImm()) // 'pred'
           .add(MI.getOperand(4))
-          .add(condCodeOp()); // 's' bit
+          .add(condCodeOp()) // 's' bit
+          .add(makeImplicit(MI.getOperand(1)));
 
       MI.eraseFromParent();
       return true;
     }
     case ARM::t2MOVCClsl:
     case ARM::t2MOVCClsr:
     case ARM::t2MOVCCasr:
     case ARM::t2MOVCCror: {
       unsigned NewOpc;
       switch (Opcode) {
       case ARM::t2MOVCClsl: NewOpc = ARM::t2LSLri; break;
       case ARM::t2MOVCClsr: NewOpc = ARM::t2LSRri; break;
       case ARM::t2MOVCCasr: NewOpc = ARM::t2ASRri; break;
       case ARM::t2MOVCCror: NewOpc = ARM::t2RORri; break;
       default: llvm_unreachable("unexpeced conditional move");
       }
       BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(NewOpc),
               MI.getOperand(1).getReg())
           .add(MI.getOperand(2))
           .addImm(MI.getOperand(3).getImm())
           .addImm(MI.getOperand(4).getImm()) // 'pred'
           .add(MI.getOperand(5))
-          .add(condCodeOp()); // 's' bit
+          .add(condCodeOp()) // 's' bit
+          .add(makeImplicit(MI.getOperand(1)));
       MI.eraseFromParent();
       return true;
     }
     case ARM::Int_eh_sjlj_dispatchsetup: {
       MachineFunction &MF = *MI.getParent()->getParent();
       const ARMBaseInstrInfo *AII =
         static_cast<const ARMBaseInstrInfo*>(TII);
       const ARMBaseRegisterInfo &RI = AII->getRegisterInfo();

lib/Target/ARM/ARMTargetMachine.cpp

extern "C" void LLVMInitializeARMTarget() {
   RegisterTargetMachine<ARMBETargetMachine> B(getTheThumbBETarget());
 
   PassRegistry &Registry = *PassRegistry::getPassRegistry();
   initializeGlobalISel(Registry);
   initializeARMLoadStoreOptPass(Registry);
   initializeARMPreAllocLoadStoreOptPass(Registry);
   initializeARMConstantIslandsPass(Registry);
   initializeARMExecutionDepsFixPass(Registry);
+  initializeARMExpandPseudoPass(Registry);
 }
 
 static std::unique_ptr<TargetLoweringObjectFile> createTLOF(const Triple &TT) {
   if (TT.isOSBinFormatMachO())
     return llvm::make_unique<TargetLoweringObjectFileMachO>();
   if (TT.isOSWindows())
     return llvm::make_unique<TargetLoweringObjectFileCOFF>();
   return llvm::make_unique<ARMElfTargetObjectFile>();

test/CodeGen/ARM/expand-pseudos.mir

+# RUN: llc -run-pass=arm-pseudo -verify-machineinstrs %s -o - | FileCheck %s
+--- |
+  target triple = "armv7---gnueabi"
+
+  define i32 @test1(i32 %x) {
+  entry:
+    unreachable
+  }
+  define i32 @test2(i32 %x) {
+  entry:
+    unreachable
+  }
+  define i32 @test3(i32 %x) {
+  entry:
+    unreachable
+  }
+...
+---
+name:            test1
+alignment:       2
+tracksRegLiveness: true
+liveins:
+  - { reg: '%r0', virtual-reg: '' }
+body:             |
+  bb.0.entry:
+    liveins: %r0
+
+    %r1 = MOVi 2, 14, _, _
+    CMPri killed %r0, 0, 14, _, implicit-def %cpsr
+    %r1 = MOVCCi16 killed %r1, 500, 0, killed %cpsr
+    %r0 = MOVr killed %r1, 14, _, _
+    BX_RET 14, _, implicit %r0
+
+...
+---
+name:            test2
+alignment:       2
+tracksRegLiveness: true
+liveins:
+  - { reg: '%r0', virtual-reg: '' }
+body:             |
+  bb.0.entry:
+    liveins: %r0
+
+    %r1 = MOVi 2, 14, _, _
+    CMPri killed %r0, 0, 14, _, implicit-def %cpsr
+    %r1 = MOVCCi32imm killed %r1, 500500500, 0, killed %cpsr
+    %r0 = MOVr killed %r1, 14, _, _
+    BX_RET 14, _, implicit %r0
+
+...
+---
+name:            test3
+alignment:       2
+tracksRegLiveness: true
+liveins:
+  - { reg: '%r0', virtual-reg: '' }
+  - { reg: '%r1', virtual-reg: '' }
+body:             |
+  bb.0.entry:
+    liveins: %r0, %r1
+
+    CMPri %r1, 500, 14, _, implicit-def %cpsr
+    %r0 = MOVCCr killed %r0, killed %r1, 12, killed %cpsr
+    BX_RET 14, _, implicit %r0
+
+...
+
+# CHECK-LABEL: name: test1
+# CHECK: %r1 = MOVi16 500, 0, killed %cpsr, implicit killed %r1
+# CHECK-LABEL: name: test2
+# CHECK:    %r1 = MOVi16 2068, 0, %cpsr, implicit killed %r1
+# CHECK:    %r1 = MOVTi16 %r1, 7637, 0, %cpsr
+# CHECK-LABEL: name: test3
+# CHECK: %r0 = MOVr killed %r1, 12, killed %cpsr, _, implicit killed %r0