[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

1081 ↗

(On Diff #105717)

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?

1135 ↗

(On Diff #105717)

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

1081 ↗

(On Diff #105717)

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.

1135 ↗

(On Diff #105717)

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

1135 ↗

(On Diff #105717)

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

test/CodeGen/ARM/expand-pseudos.mir

69 ↗

(On Diff #106109)

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.

llvm/trunk/lib/Target/ARM/ARMExpandPseudoInsts.cpp

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();

llvm/trunk/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