[ARM] Use helpers for adding pred / CC operands. NFC

[rovka]

Summary

Hunt down some of the places where we use bare addReg(0) or addImm(AL).addReg(0)
and replace with add(condCodeOp()) and add(predOps()). This should make it
easier to understand what those operands represent (without having to look at
the definition of the instruction that we're adding to).

[rengolin]

Hi Diana,

I have to say I never really liked the AddDefaultPred wrappers, so adding a new one for CC strikes me as going in the wrong direction.

I very much like the builder pattern as in RFC2. I think we can focus in the lowering layer for now and worry about MC later.

cheers,
--renato

[rovka]

Thanks!

I'll have a shot at RFC 2.

FWIW, the RFCs are mostly orthogonal, so if we implement RFC 2 we can still do RFC 1 and / or RFC 3 on top of it:

• RFC 1: our ARM builder will just have an addPred method instead of addDefaultPred
• RFC 3: we can still have a template <typename WrappedBuilder> class ARMBuilder : public <WrappedBuilder>

Naturally, I would split all of that up into several patches.

[kristof.beyls]

I do think that using addDefaultPred/CC makes the code more readable compared to using .addreg(0) or .addImm((int64_t)ARMCC::AL).addReg(0), especially for newcomers to the ARM backend: it makes the BuildMI lines look that little bit more in line with ARM assembly syntax. In other words, it is easier to guess what "AddDefaultPred" or "AddDefaultCC" mean than ".addreg(0)", when the ARM assembly syntax for the corresponding instruction doesn't have that extra operand...

I don't have a strong preference over using addDefaultPred/CC or the other alternative discussed syntaxes like MIB.addSomeOperand().addSomeOtherOperand().addDefaultPred(), but I do think that the existing addDefaultPred/CC syntax may keep the BuildMI lines a little bit more similar to the ARM assembly syntax for the instruction that is being constructed: i.e. the Pred and CC parts are not written as normal operands to instructions.
I guess we'd have to compare a substantial sample of all the lines of code where this pattern can be applied to see if this similarity between the assembly syntax of the instruction and the BuildMI syntax actually holds; or if it's just hand-wavy wishful thinking on my part.

Trying to do that exercise on one of the lines changed in this patch:

AddDefaultCC(
    AddDefaultPred(BuildMI(MBB, MBBI, dl, TII.get(ARM::MOVr), ARM::SP)
                       .addReg(FramePtr)));

corresponds, I think, with the following instruction:

MOV SP, FramePtr.

There is a clearer mapping between the BuildMI statement and the corresponding assembly instruction, I think, than with the alternative syntaxes discussed earlier on this review.

TBH, if we'd decide to stick to using addDefaultPred/CC, these functions could use some doxygen documentation.

[kristof.beyls]

Thinking a bit more about this, if we'd end up deciding to go for option 2, I think the following function names and arguments would make code more easily readable compared to .addDefaultPred() or .addDefaultCC():

MIB.addSomeOperand().addSomeOtherOperand().addCondCode(ARMCC::AL).isFlagSetting(false)

i.e. drop the defaults (adding the explicit value ARMCC::AL or false is about as many characters as the word "Default", and it's more obvious what the meaning is of the code. Furthermore, for non-default values of these operands, also the more meaningful function names can be used, e.g. addCondCode(ARMCC:EQ) or isFlagSetting(true).

[MatzeB]

First: Factoring out the common code for adding those operands and thereby giving it a more obvious name than addReg(0) is a good thing!

I guess we'd have to compare a substantial sample of all the lines of code where this pattern can be applied to see if this similarity between the assembly syntax of the instruction and the BuildMI syntax actually holds; or if it's just hand-wavy wishful thinking on my part.

We should really compare the BuildMI order with how the MachineInstrs will look later! Of course it is a good idea to have the operand order of the MachineInstrs correspond to assembly syntax, but having BuildMI correspond to assembly doesn't help if the MachineInstrs themselfes have a different order.

I'm not necessarily against the custom MachineInstrBuilder, but all the templating machinery
feels a bit heavy for the problem at hand so I proposed a variant on how to integrate this with BuildMI in https://reviews.llvm.org/D28057.

[rovka]

Hi,

Thanks to everyone for the suggestions.
Would something like this [1] do the trick? I haven't used a pair because from the MachineInstrBuilder's POV there's nothing special about pairs of MachineOperands. I also replaced all uses here [2] - I did this automatically with a custom tool, so I can easily change all uses again if people think it's still ugly.

[1] https://reviews.llvm.org/differential/diff/83787/
[2] https://reviews.llvm.org/differential/diff/83788/

[rovka]

Update patch to use the new helpers from r291890 and r291893.
With this, RFC 1 and RFC 2 are fully implemented (modulo any uses that I may have missed).
I'll leave RFC 3 (MCInstBuilder) for another time.

[rengolin]

Much better!

[rovka]

Ping.

[rengolin]

Ok, this is just finishing off the pattern that was agreed and implemented beforehand, so I'll approve. LGTM. Thanks!

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

if (OffReg == 0) {
       if (ARM_AM::getSOImmVal(Amt) == -1)
         // Can't encode it in a so_imm operand. This transformation will
         // add more than 1 instruction. Abandon!
         return nullptr;
       UpdateMI = BuildMI(MF, MI.getDebugLoc(),
                          get(isSub ? ARM::SUBri : ARM::ADDri), WBReg)
                      .addReg(BaseReg)
                      .addImm(Amt)
-                     .addImm(Pred)
-                     .addReg(0)
-                     .addReg(0);
+                     .add(predOps(Pred))
+                     .add(condCodeOp());
     } else if (Amt != 0) {
       ARM_AM::ShiftOpc ShOpc = ARM_AM::getAM2ShiftOpc(OffImm);
       unsigned SOOpc = ARM_AM::getSORegOpc(ShOpc, Amt);
       UpdateMI = BuildMI(MF, MI.getDebugLoc(),
                          get(isSub ? ARM::SUBrsi : ARM::ADDrsi), WBReg)
                      .addReg(BaseReg)
                      .addReg(OffReg)
                      .addReg(0)
                      .addImm(SOOpc)
-                     .addImm(Pred)
-                     .addReg(0)
-                     .addReg(0);
+                     .add(predOps(Pred))
+                     .add(condCodeOp());
     } else
       UpdateMI = BuildMI(MF, MI.getDebugLoc(),
                          get(isSub ? ARM::SUBrr : ARM::ADDrr), WBReg)
                      .addReg(BaseReg)
                      .addReg(OffReg)
-                     .addImm(Pred)
-                     .addReg(0)
-                     .addReg(0);
+                     .add(predOps(Pred))
+                     .add(condCodeOp());
     break;
   }
   case ARMII::AddrMode3 : {
     bool isSub = ARM_AM::getAM3Op(OffImm) == ARM_AM::sub;
     unsigned Amt = ARM_AM::getAM3Offset(OffImm);
     if (OffReg == 0)
       // Immediate is 8-bits. It's guaranteed to fit in a so_imm operand.
       UpdateMI = BuildMI(MF, MI.getDebugLoc(),
                          get(isSub ? ARM::SUBri : ARM::ADDri), WBReg)
                      .addReg(BaseReg)
                      .addImm(Amt)
-                     .addImm(Pred)
-                     .addReg(0)
-                     .addReg(0);
+                     .add(predOps(Pred))
+                     .add(condCodeOp());
     else
       UpdateMI = BuildMI(MF, MI.getDebugLoc(),
                          get(isSub ? ARM::SUBrr : ARM::ADDrr), WBReg)
                      .addReg(BaseReg)
                      .addReg(OffReg)
-                     .addImm(Pred)
-                     .addReg(0)
-                     .addReg(0);
+                     .add(predOps(Pred))
+                     .add(condCodeOp());
     break;
   }
   }
 
   std::vector<MachineInstr*> NewMIs;
   if (isPre) {
     if (isLoad)
       MemMI =
unsigned ARMBaseInstrInfo::insertBranch(MachineBasicBlock &MBB,
   assert((Cond.size() == 2 || Cond.size() == 0) &&
          "ARM branch conditions have two components!");
 
   // For conditional branches, we use addOperand to preserve CPSR flags.
 
   if (!FBB) {
     if (Cond.empty()) { // Unconditional branch?
       if (isThumb)
-        BuildMI(&MBB, DL, get(BOpc)).addMBB(TBB).addImm(ARMCC::AL).addReg(0);
+        BuildMI(&MBB, DL, get(BOpc)).addMBB(TBB).add(predOps(ARMCC::AL));
       else
         BuildMI(&MBB, DL, get(BOpc)).addMBB(TBB);
     } else
       BuildMI(&MBB, DL, get(BccOpc))
           .addMBB(TBB)
           .addImm(Cond[0].getImm())
           .add(Cond[1]);
     return 1;
   }
 
   // Two-way conditional branch.
   BuildMI(&MBB, DL, get(BccOpc))
       .addMBB(TBB)
       .addImm(Cond[0].getImm())
       .add(Cond[1]);
   if (isThumb)
-    BuildMI(&MBB, DL, get(BOpc)).addMBB(FBB).addImm(ARMCC::AL).addReg(0);
+    BuildMI(&MBB, DL, get(BOpc)).addMBB(FBB).add(predOps(ARMCC::AL));
   else
     BuildMI(&MBB, DL, get(BOpc)).addMBB(FBB);
   return 2;
 }
 
 bool ARMBaseInstrInfo::
 reverseBranchCondition(SmallVectorImpl<MachineOperand> &Cond) const {
   ARMCC::CondCodes CC = (ARMCC::CondCodes)(int)Cond[0].getImm();
void llvm::emitARMRegPlusImmediate(MachineBasicBlock &MBB,
                                    MachineBasicBlock::iterator &MBBI,
                                    const DebugLoc &dl, unsigned DestReg,
                                    unsigned BaseReg, int NumBytes,
                                    ARMCC::CondCodes Pred, unsigned PredReg,
                                    const ARMBaseInstrInfo &TII,
                                    unsigned MIFlags) {
   if (NumBytes == 0 && DestReg != BaseReg) {
     BuildMI(MBB, MBBI, dl, TII.get(ARM::MOVr), DestReg)
         .addReg(BaseReg, RegState::Kill)
-      .addImm((unsigned)Pred).addReg(PredReg).addReg(0)
+        .add(predOps(Pred, PredReg))
+        .add(condCodeOp())
         .setMIFlags(MIFlags);
     return;
   }
 
   bool isSub = NumBytes < 0;
   if (isSub) NumBytes = -NumBytes;
 
   while (NumBytes) {
     unsigned RotAmt = ARM_AM::getSOImmValRotate(NumBytes);
     unsigned ThisVal = NumBytes & ARM_AM::rotr32(0xFF, RotAmt);
     assert(ThisVal && "Didn't extract field correctly");
 
     // We will handle these bits from offset, clear them.
     NumBytes &= ~ThisVal;
 
     assert(ARM_AM::getSOImmVal(ThisVal) != -1 && "Bit extraction didn't work?");
 
     // Build the new ADD / SUB.
     unsigned Opc = isSub ? ARM::SUBri : ARM::ADDri;
     BuildMI(MBB, MBBI, dl, TII.get(Opc), DestReg)
-      .addReg(BaseReg, RegState::Kill).addImm(ThisVal)
-      .addImm((unsigned)Pred).addReg(PredReg).addReg(0)
+        .addReg(BaseReg, RegState::Kill)
+        .addImm(ThisVal)
+        .add(predOps(Pred, PredReg))
+        .add(condCodeOp())
         .setMIFlags(MIFlags);
     BaseReg = DestReg;
   }
 }
 
 bool llvm::tryFoldSPUpdateIntoPushPop(const ARMSubtarget &Subtarget,
                                       MachineFunction &MF, MachineInstr *MI,
                                       unsigned NumBytes) {
   // This optimisation potentially adds lots of load and store

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

void ARMBaseRegisterInfo::emitLoadConstPool(
   MachineFunction &MF = *MBB.getParent();
   const TargetInstrInfo &TII = *MF.getSubtarget().getInstrInfo();
   MachineConstantPool *ConstantPool = MF.getConstantPool();
   const Constant *C =
         ConstantInt::get(Type::getInt32Ty(MF.getFunction()->getContext()), Val);
   unsigned Idx = ConstantPool->getConstantPoolIndex(C, 4);
 
   BuildMI(MBB, MBBI, dl, TII.get(ARM::LDRcp))
       .addReg(DestReg, getDefRegState(true), SubIdx)
       .addConstantPoolIndex(Idx)
-    .addImm(0).addImm(Pred).addReg(PredReg)
+      .addImm(0)
+      .add(predOps(Pred, PredReg))
       .setMIFlags(MIFlags);
 }
 
 bool ARMBaseRegisterInfo::
 requiresRegisterScavenging(const MachineFunction &MF) const {
   return true;
 }
 
 bool ARMBaseRegisterInfo::

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

MachineBasicBlock *ARMConstantIslands::splitBlockBeforeInstr(MachineInstr *MI) {
   // Add an unconditional branch from OrigBB to NewBB.
   // Note the new unconditional branch is not being recorded.
   // There doesn't seem to be meaningful DebugInfo available; this doesn't
   // correspond to anything in the source.
   unsigned Opc = isThumb ? (isThumb2 ? ARM::t2B : ARM::tB) : ARM::B;
   if (!isThumb)
     BuildMI(OrigBB, DebugLoc(), TII->get(Opc)).addMBB(NewBB);
   else
-    BuildMI(OrigBB, DebugLoc(), TII->get(Opc)).addMBB(NewBB)
-            .addImm(ARMCC::AL).addReg(0);
+    BuildMI(OrigBB, DebugLoc(), TII->get(Opc))
+        .addMBB(NewBB)
+        .add(predOps(ARMCC::AL));
   ++NumSplit;
 
   // Update the CFG.  All succs of OrigBB are now succs of NewBB.
   NewBB->transferSuccessors(OrigBB);
 
   // OrigBB branches to NewBB.
   OrigBB->addSuccessor(NewBB);
 
if (isOffsetInRange(UserOffset, CPEOffset, U)) {
       // it for branch lengthening; this new branch will not get out of range,
       // but if the preceding conditional branch is out of range, the targets
       // will be exchanged, and the altered branch may be out of range, so the
       // machinery has to know about it.
       int UncondBr = isThumb ? ((isThumb2) ? ARM::t2B : ARM::tB) : ARM::B;
       if (!isThumb)
         BuildMI(UserMBB, DebugLoc(), TII->get(UncondBr)).addMBB(NewMBB);
       else
-        BuildMI(UserMBB, DebugLoc(), TII->get(UncondBr)).addMBB(NewMBB)
-          .addImm(ARMCC::AL).addReg(0);
+        BuildMI(UserMBB, DebugLoc(), TII->get(UncondBr))
+            .addMBB(NewMBB)
+            .add(predOps(ARMCC::AL));
       unsigned MaxDisp = getUnconditionalBrDisp(UncondBr);
       ImmBranches.push_back(ImmBranch(&UserMBB->back(),
                                       MaxDisp, false, UncondBr));
       computeBlockSize(MF, UserMBB, BBInfo[UserMBB->getNumber()]);
       adjustBBOffsetsAfter(UserMBB);
       return;
     }
   }
ARMConstantIslands::fixupConditionalBr(ImmBranch &Br) {
 
   // Insert a new conditional branch and a new unconditional branch.
   // Also update the ImmBranch as well as adding a new entry for the new branch.
   BuildMI(MBB, DebugLoc(), TII->get(MI->getOpcode()))
     .addMBB(NextBB).addImm(CC).addReg(CCReg);
   Br.MI = &MBB->back();
   BBInfo[MBB->getNumber()].Size += TII->getInstSizeInBytes(MBB->back());
   if (isThumb)
-    BuildMI(MBB, DebugLoc(), TII->get(Br.UncondBr)).addMBB(DestBB)
-            .addImm(ARMCC::AL).addReg(0);
+    BuildMI(MBB, DebugLoc(), TII->get(Br.UncondBr))
+        .addMBB(DestBB)
+        .add(predOps(ARMCC::AL));
   else
     BuildMI(MBB, DebugLoc(), TII->get(Br.UncondBr)).addMBB(DestBB);
   BBInfo[MBB->getNumber()].Size += TII->getInstSizeInBytes(MBB->back());
   unsigned MaxDisp = getUnconditionalBrDisp(Br.UncondBr);
   ImmBranches.push_back(ImmBranch(&MBB->back(), MaxDisp, false, Br.UncondBr));
 
   // Remove the old conditional branch.  It may or may not still be in MBB.
   BBInfo[MI->getParent()->getNumber()].Size -= TII->getInstSizeInBytes(*MI);
adjustJTTargetBlockForward(MachineBasicBlock *BB, MachineBasicBlock *JTBB) {
   MF->insert(MBBI, NewBB);
 
   // Add an unconditional branch from NewBB to BB.
   // There doesn't seem to be meaningful DebugInfo available; this doesn't
   // correspond directly to anything in the source.
   if (isThumb2)
     BuildMI(NewBB, DebugLoc(), TII->get(ARM::t2B))
         .addMBB(BB)
-        .addImm(ARMCC::AL)
-        .addReg(0);
+        .add(predOps(ARMCC::AL));
   else
     BuildMI(NewBB, DebugLoc(), TII->get(ARM::tB))
         .addMBB(BB)
-        .addImm(ARMCC::AL)
-        .addReg(0);
+        .add(predOps(ARMCC::AL));
 
   // Update internal data structures to account for the newly inserted MBB.
   MF->RenumberBlocks(NewBB);
 
   // Update the CFG.
   NewBB->addSuccessor(BB);
   JTBB->replaceSuccessor(BB, NewBB);
 
   ++NumJTInserted;
   return NewBB;
 }

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

if (!STI->hasV6T2Ops() &&
     assert (MO.isImm() && "MOVi32imm w/ non-immediate source operand!");
     unsigned ImmVal = (unsigned)MO.getImm();
     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).addReg(0);
-    HI16.addImm(Pred).addReg(PredReg).addReg(0);
+    LO16.addImm(Pred).addReg(PredReg).add(condCodeOp());
+    HI16.addImm(Pred).addReg(PredReg).add(condCodeOp());
     TransferImpOps(MI, LO16, HI16);
     MI.eraseFromParent();
     return;
   }
 
   unsigned LO16Opc = 0;
   unsigned HI16Opc = 0;
   if (Opcode == ARM::t2MOVi32imm || Opcode == ARM::t2MOVCCi32imm) {
case ARM::TCRETURNri: {
         else {
           assert(JumpTarget.isSymbol());
           MIB.addExternalSymbol(JumpTarget.getSymbolName(),
                                 JumpTarget.getTargetFlags());
         }
 
         // Add the default predicate in Thumb mode.
         if (STI->isThumb())
-          MIB.addImm(ARMCC::AL).addReg(0);
+          MIB.add(predOps(ARMCC::AL));
       } else if (RetOpcode == ARM::TCRETURNri) {
         BuildMI(MBB, MBBI, dl,
                 TII.get(STI->isThumb() ? ARM::tTAILJMPr : ARM::TAILJMPr))
             .addReg(JumpTarget.getReg(), RegState::Kill);
       }
 
       auto NewMI = std::prev(MBBI);
       for (unsigned i = 1, e = MBBI->getNumOperands(); i != e; ++i)
switch (Opcode) {
     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))
-          .addReg(0); // 's' bit
+          .add(condCodeOp()); // 's' bit
 
       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))
-          .addReg(0); // 's' bit
+          .add(condCodeOp()); // 's' bit
 
       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))
-          .addReg(0); // 's' bit
+          .add(condCodeOp()); // 's' bit
 
       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));
       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))
-          .addReg(0); // 's' bit
+          .add(condCodeOp()); // 's' bit
 
       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))
-          .addReg(0); // 's' bit
+          .add(condCodeOp()); // 's' bit
 
       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))
-          .addReg(0); // 's' bit
+          .add(condCodeOp()); // 's' bit
       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();
switch (Opcode) {
     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))
-              .addReg(0);
+              .add(condCodeOp());
       TransferImpOps(MI, MIB, MIB);
       MI.eraseFromParent();
       return true;
     }
     case ARM::tTPsoft:
     case ARM::TPsoft: {
       MachineInstrBuilder MIB;
       if (Opcode == ARM::tTPsoft)
-        MIB = BuildMI(MBB, MBBI, MI.getDebugLoc(),
-                      TII->get( ARM::tBL))
-              .addImm((unsigned)ARMCC::AL).addReg(0)
+        MIB = BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(ARM::tBL))
+                  .add(predOps(ARMCC::AL))
                   .addExternalSymbol("__aeabi_read_tp", 0);
       else
         MIB = BuildMI(MBB, MBBI, MI.getDebugLoc(),
                       TII->get( ARM::BL))
               .addExternalSymbol("__aeabi_read_tp", 0);
 
       MIB->setMemRefs(MI.memoperands_begin(), MI.memoperands_end());
       TransferImpOps(MI, MIB, MIB);
       MI.eraseFromParent();

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

else
         .setMIFlags(MachineInstr::FrameSetup);
 
     switch (TM.getCodeModel()) {
     case CodeModel::Small:
     case CodeModel::Medium:
     case CodeModel::Default:
     case CodeModel::Kernel:
       BuildMI(MBB, MBBI, dl, TII.get(ARM::tBL))
-        .addImm((unsigned)ARMCC::AL).addReg(0)
+          .add(predOps(ARMCC::AL))
           .addExternalSymbol("__chkstk")
           .addReg(ARM::R4, RegState::Implicit)
           .setMIFlags(MachineInstr::FrameSetup);
       break;
     case CodeModel::Large:
     case CodeModel::JITDefault:
       BuildMI(MBB, MBBI, dl, TII.get(ARM::t2MOVi32imm), ARM::R12)
         .addExternalSymbol("__chkstk")
         .setMIFlags(MachineInstr::FrameSetup);
 
       BuildMI(MBB, MBBI, dl, TII.get(ARM::tBLXr))
-        .addImm((unsigned)ARMCC::AL).addReg(0)
+          .add(predOps(ARMCC::AL))
           .addReg(ARM::R12, RegState::Kill)
           .addReg(ARM::R4, RegState::Implicit)
           .setMIFlags(MachineInstr::FrameSetup);
       break;
     }
 
     BuildMI(MBB, MBBI, dl, TII.get(ARM::t2SUBrr), ARM::SP)
         .addReg(ARM::SP, RegState::Kill)
         .addReg(ARM::R4, RegState::Kill)
         .setMIFlags(MachineInstr::FrameSetup)
         .add(predOps(ARMCC::AL))
void ARMFrameLowering::emitPrologue(MachineFunction &MF,
 
   // If we need a base pointer, set it up here. It's whatever the value
   // of the stack pointer is at this point. Any variable size objects
   // will be allocated after this, so we can still use the base pointer
   // to reference locals.
   // FIXME: Clarify FrameSetup flags here.
   if (RegInfo->hasBasePointer(MF)) {
     if (isARM)
-      BuildMI(MBB, MBBI, dl,
-              TII.get(ARM::MOVr), RegInfo->getBaseRegister())
+      BuildMI(MBB, MBBI, dl, TII.get(ARM::MOVr), RegInfo->getBaseRegister())
           .addReg(ARM::SP)
-        .addImm((unsigned)ARMCC::AL).addReg(0).addReg(0);
+          .add(predOps(ARMCC::AL))
+          .add(condCodeOp());
     else
       BuildMI(MBB, MBBI, dl, TII.get(ARM::tMOVr), RegInfo->getBaseRegister())
           .addReg(ARM::SP)
           .add(predOps(ARMCC::AL));
   }
 
   // If the frame has variable sized objects then the epilogue must restore
   // the sp from fp. We can assume there's an FP here since hasFP already
if (AFI->shouldRestoreSPFromFP()) {
           BuildMI(MBB, MBBI, dl, TII.get(ARM::tMOVr), ARM::SP)
               .addReg(ARM::R4)
               .add(predOps(ARMCC::AL));
         }
       } else {
         // Thumb2 or ARM.
         if (isARM)
           BuildMI(MBB, MBBI, dl, TII.get(ARM::MOVr), ARM::SP)
-            .addReg(FramePtr).addImm((unsigned)ARMCC::AL).addReg(0).addReg(0);
+              .addReg(FramePtr)
+              .add(predOps(ARMCC::AL))
+              .add(condCodeOp());
         else
           BuildMI(MBB, MBBI, dl, TII.get(ARM::tMOVr), ARM::SP)
               .addReg(FramePtr)
               .add(predOps(ARMCC::AL));
       }
     } else if (NumBytes &&
                !tryFoldSPUpdateIntoPushPop(STI, MF, &*MBBI, NumBytes))
       emitSPUpdate(isARM, MBB, MBBI, dl, TII, NumBytes);
void ARMFrameLowering::adjustForSegmentedStacks(
   if (Thumb) {
     BuildMI(McrMBB, DL, TII.get(ARM::tMOVr), ScratchReg1)
         .addReg(ARM::SP)
         .add(predOps(ARMCC::AL));
   } else if (CompareStackPointer) {
     BuildMI(McrMBB, DL, TII.get(ARM::MOVr), ScratchReg1)
         .addReg(ARM::SP)
         .add(predOps(ARMCC::AL))
-        .addReg(0);
+        .add(condCodeOp());
   }
 
   // sub SR1, sp, #StackSize
   if (!CompareStackPointer && Thumb) {
     BuildMI(McrMBB, DL, TII.get(ARM::tSUBi8), ScratchReg1)
         .add(condCodeOp())
         .addReg(ScratchReg1)
         .addImm(AlignedStackSize)
         .add(predOps(ARMCC::AL));
   } else if (!CompareStackPointer) {
     BuildMI(McrMBB, DL, TII.get(ARM::SUBri), ScratchReg1)
         .addReg(ARM::SP)
         .addImm(AlignedStackSize)
         .add(predOps(ARMCC::AL))
-        .addReg(0);
+        .add(condCodeOp());
   }
 
   if (Thumb && ST->isThumb1Only()) {
     unsigned PCLabelId = ARMFI->createPICLabelUId();
     ARMConstantPoolValue *NewCPV = ARMConstantPoolSymbol::Create(
         MF.getFunction()->getContext(), "__STACK_LIMIT", PCLabelId, 0);
     MachineConstantPool *MCP = MF.getConstantPool();
     unsigned CPI = MCP->getConstantPoolIndex(NewCPV, 4);
if (Thumb) {
     BuildMI(AllocMBB, DL, TII.get(ARM::tMOVi8), ScratchReg0)
         .add(condCodeOp())
         .addImm(AlignedStackSize)
         .add(predOps(ARMCC::AL));
   } else {
     BuildMI(AllocMBB, DL, TII.get(ARM::MOVi), ScratchReg0)
         .addImm(AlignedStackSize)
         .add(predOps(ARMCC::AL))
-        .addReg(0);
+        .add(condCodeOp());
   }
   // Pass second argument for the __morestack by Scratch Register #1.
   //   The amount size of stack consumed to save function arguments.
   if (Thumb) {
     BuildMI(AllocMBB, DL, TII.get(ARM::tMOVi8), ScratchReg1)
         .add(condCodeOp())
         .addImm(alignToARMConstant(ARMFI->getArgumentStackSize()))
         .add(predOps(ARMCC::AL));
   } else {
     BuildMI(AllocMBB, DL, TII.get(ARM::MOVi), ScratchReg1)
         .addImm(alignToARMConstant(ARMFI->getArgumentStackSize()))
         .add(predOps(ARMCC::AL))
-        .addReg(0);
+        .add(condCodeOp());
   }
 
   // push {lr} - Save return address of this function.
   if (Thumb) {
     BuildMI(AllocMBB, DL, TII.get(ARM::tPUSH))
         .add(predOps(ARMCC::AL))
         .addReg(ARM::LR);
   } else {

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

ARMTargetLowering::EmitLowered__chkstk(MachineInstr &MI,
   // IP.
 
   switch (TM.getCodeModel()) {
   case CodeModel::Small:
   case CodeModel::Medium:
   case CodeModel::Default:
   case CodeModel::Kernel:
     BuildMI(*MBB, MI, DL, TII.get(ARM::tBL))
-      .addImm((unsigned)ARMCC::AL).addReg(0)
+        .add(predOps(ARMCC::AL))
         .addExternalSymbol("__chkstk")
         .addReg(ARM::R4, RegState::Implicit | RegState::Kill)
         .addReg(ARM::R4, RegState::Implicit | RegState::Define)
-      .addReg(ARM::R12, RegState::Implicit | RegState::Define | RegState::Dead);
+        .addReg(ARM::R12,
+                RegState::Implicit | RegState::Define | RegState::Dead);
     break;
   case CodeModel::Large:
   case CodeModel::JITDefault: {
     MachineRegisterInfo &MRI = MBB->getParent()->getRegInfo();
     unsigned Reg = MRI.createVirtualRegister(&ARM::rGPRRegClass);
 
     BuildMI(*MBB, MI, DL, TII.get(ARM::t2MOVi32imm), Reg)
       .addExternalSymbol("__chkstk");
     BuildMI(*MBB, MI, DL, TII.get(ARM::tBLXr))
-      .addImm((unsigned)ARMCC::AL).addReg(0)
+        .add(predOps(ARMCC::AL))
         .addReg(Reg, RegState::Kill)
         .addReg(ARM::R4, RegState::Implicit | RegState::Kill)
         .addReg(ARM::R4, RegState::Implicit | RegState::Define)
-      .addReg(ARM::R12, RegState::Implicit | RegState::Define | RegState::Dead);
+        .addReg(ARM::R12,
+                RegState::Implicit | RegState::Define | RegState::Dead);
     break;
   }
   }
 
   BuildMI(*MBB, MI, DL, TII.get(ARM::t2SUBrr), ARM::SP)
       .addReg(ARM::SP, RegState::Kill)
       .addReg(ARM::R4, RegState::Kill)
       .setMIFlags(MachineInstr::FrameSetup)
case ARM::t2ABS: {
     BuildMI(BB, dl,
       TII->get(isThumb2 ? ARM::t2Bcc : ARM::Bcc)).addMBB(SinkBB)
       .addImm(ARMCC::getOppositeCondition(ARMCC::MI)).addReg(ARM::CPSR);
 
     // insert rsbri in RSBBB
     // Note: BCC and rsbri will be converted into predicated rsbmi
     // by if-conversion pass
     BuildMI(*RSBBB, RSBBB->begin(), dl,
             TII->get(isThumb2 ? ARM::t2RSBri : ARM::RSBri), NewRsbDstReg)
         .addReg(ABSSrcReg, ABSSrcKIll ? RegState::Kill : 0)
-      .addImm(0).addImm((unsigned)ARMCC::AL).addReg(0).addReg(0);
+        .addImm(0)
+        .add(predOps(ARMCC::AL))
+        .add(condCodeOp());
 
     // insert PHI in SinkBB,
     // reuse ABSDstReg to not change uses of ABS instruction
     BuildMI(*SinkBB, SinkBB->begin(), dl,
       TII->get(ARM::PHI), ABSDstReg)
       .addReg(NewRsbDstReg).addMBB(RSBBB)
       .addReg(ABSSrcReg).addMBB(BB);
 

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

if (isThumb1) {
             !STI->hasV6Ops()) {
           // thumbv4t doesn't have lo->lo copies, and we can't predicate tMOVSr
           if (Pred != ARMCC::AL)
             return nullptr;
           BuildMI(MBB, InsertBefore, DL, TII->get(ARM::tMOVSr), NewBase)
             .addReg(Base, getKillRegState(KillOldBase));
         } else
           BuildMI(MBB, InsertBefore, DL, TII->get(ARM::tMOVr), NewBase)
               .addReg(Base, getKillRegState(KillOldBase))
-            .addImm(Pred).addReg(PredReg);
+              .add(predOps(Pred, PredReg));
 
         // The following ADDS/SUBS becomes an update.
         Base = NewBase;
         KillOldBase = true;
       }
       if (BaseOpc == ARM::tADDrSPi) {
         assert(Offset % 4 == 0 && "tADDrSPi offset is scaled by 4");
         BuildMI(MBB, InsertBefore, DL, TII->get(BaseOpc), NewBase)
-          .addReg(Base, getKillRegState(KillOldBase)).addImm(Offset/4)
-          .addImm(Pred).addReg(PredReg);
+            .addReg(Base, getKillRegState(KillOldBase))
+            .addImm(Offset / 4)
+            .add(predOps(Pred, PredReg));
       } else
         BuildMI(MBB, InsertBefore, DL, TII->get(BaseOpc), NewBase)
             .add(t1CondCodeOp(true))
             .addReg(Base, getKillRegState(KillOldBase))
             .addImm(Offset)
-            .addImm(Pred)
-            .addReg(PredReg);
+            .add(predOps(Pred, PredReg));
     } else {
       BuildMI(MBB, InsertBefore, DL, TII->get(BaseOpc), NewBase)
-        .addReg(Base, getKillRegState(KillOldBase)).addImm(Offset)
-        .addImm(Pred).addReg(PredReg).addReg(0);
+          .addReg(Base, getKillRegState(KillOldBase))
+          .addImm(Offset)
+          .add(predOps(Pred, PredReg))
+          .add(condCodeOp());
     }
     Base = NewBase;
     BaseKill = true; // New base is always killed straight away.
   }
 
   bool isDef = isLoadSingle(Opcode);
 
   // Get LS multiple opcode. Note that for Thumb1 this might be an opcode with
if (isAM2) {
       // LDR_PRE, LDR_POST
       if (NewOpc == ARM::LDR_PRE_IMM || NewOpc == ARM::LDRB_PRE_IMM) {
         BuildMI(MBB, MBBI, DL, TII->get(NewOpc), MI->getOperand(0).getReg())
           .addReg(Base, RegState::Define)
           .addReg(Base).addImm(Offset).addImm(Pred).addReg(PredReg);
       } else {
         int Imm = ARM_AM::getAM2Opc(AddSub, Bytes, ARM_AM::no_shift);
         BuildMI(MBB, MBBI, DL, TII->get(NewOpc), MI->getOperand(0).getReg())
             .addReg(Base, RegState::Define)
-          .addReg(Base).addReg(0).addImm(Imm).addImm(Pred).addReg(PredReg);
+            .addReg(Base)
+            .addReg(0)
+            .addImm(Imm)
+            .add(predOps(Pred, PredReg));
       }
     } else {
       // t2LDR_PRE, t2LDR_POST
       BuildMI(MBB, MBBI, DL, TII->get(NewOpc), MI->getOperand(0).getReg())
           .addReg(Base, RegState::Define)
-        .addReg(Base).addImm(Offset).addImm(Pred).addReg(PredReg);
+          .addReg(Base)
+          .addImm(Offset)
+          .add(predOps(Pred, PredReg));
     }
   } else {
     MachineOperand &MO = MI->getOperand(0);
     // FIXME: post-indexed stores use am2offset_imm, which still encodes
     // the vestigal zero-reg offset register. When that's fixed, this clause
     // can be removed entirely.
     if (isAM2 && NewOpc == ARM::STR_POST_IMM) {
       int Imm = ARM_AM::getAM2Opc(AddSub, Bytes, ARM_AM::no_shift);
       // STR_PRE, STR_POST
       BuildMI(MBB, MBBI, DL, TII->get(NewOpc), Base)
           .addReg(MO.getReg(), getKillRegState(MO.isKill()))
-        .addReg(Base).addReg(0).addImm(Imm).addImm(Pred).addReg(PredReg);
+          .addReg(Base)
+          .addReg(0)
+          .addImm(Imm)
+          .add(predOps(Pred, PredReg));
     } else {
       // t2STR_PRE, t2STR_POST
       BuildMI(MBB, MBBI, DL, TII->get(NewOpc), Base)
           .addReg(MO.getReg(), getKillRegState(MO.isKill()))
-        .addReg(Base).addImm(Offset).addImm(Pred).addReg(PredReg);
+          .addReg(Base)
+          .addImm(Offset)
+          .add(predOps(Pred, PredReg));
     }
   }
   MBB.erase(MBBI);
 
   return true;
 }
 
 bool ARMLoadStoreOpt::MergeBaseUpdateLSDouble(MachineInstr &MI) const {

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

if (NumBytes < 65536) {
         .addImm(NumBytes >> 16)
         .addImm((unsigned)Pred).addReg(PredReg).setMIFlags(MIFlags);
       Fits = true;
     }
 
     if (Fits) {
       if (isSub) {
         BuildMI(MBB, MBBI, dl, TII.get(ARM::t2SUBrr), DestReg)
             .addReg(BaseReg)
             .addReg(DestReg, RegState::Kill)
-          .addImm((unsigned)Pred).addReg(PredReg).addReg(0)
+            .add(predOps(Pred, PredReg))
+            .add(condCodeOp())
             .setMIFlags(MIFlags);
       } else {
         // Here we know that DestReg is not SP but we do not
         // know anything about BaseReg. t2ADDrr is an invalid
         // instruction is SP is used as the second argument, but
         // is fine if SP is the first argument. To be sure we
         // do not generate invalid encoding, put BaseReg first.
         BuildMI(MBB, MBBI, dl, TII.get(ARM::t2ADDrr), DestReg)
             .addReg(BaseReg)
             .addReg(DestReg, RegState::Kill)
-          .addImm((unsigned)Pred).addReg(PredReg).addReg(0)
+            .add(predOps(Pred, PredReg))
+            .add(condCodeOp())
             .setMIFlags(MIFlags);
       }
       return;
     }
   }
 
   while (NumBytes) {
     unsigned ThisVal = NumBytes;
     unsigned Opc = 0;

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

static void emitThumb2LoadConstPool(MachineBasicBlock &MBB,
   MachineFunction &MF = *MBB.getParent();
   const TargetInstrInfo &TII = *MF.getSubtarget().getInstrInfo();
   MachineConstantPool *ConstantPool = MF.getConstantPool();
   const Constant *C = ConstantInt::get(
            Type::getInt32Ty(MBB.getParent()->getFunction()->getContext()), Val);
   unsigned Idx = ConstantPool->getConstantPoolIndex(C, 4);
 
   BuildMI(MBB, MBBI, dl, TII.get(ARM::t2LDRpci))
       .addReg(DestReg, getDefRegState(true), SubIdx)
-    .addConstantPoolIndex(Idx).addImm((int64_t)ARMCC::AL).addReg(0)
+      .addConstantPoolIndex(Idx)
+      .add(predOps(ARMCC::AL))
       .setMIFlags(MIFlags);
 }
 
 /// emitLoadConstPool - Emits a load from constpool to materialize the
 /// specified immediate.
 void ThumbRegisterInfo::emitLoadConstPool(
     MachineBasicBlock &MBB, MachineBasicBlock::iterator &MBBI,
     const DebugLoc &dl, unsigned DestReg, unsigned SubIdx, int Val,
     ARMCC::CondCodes Pred, unsigned PredReg, unsigned MIFlags) const {