[MachineInstruction] Put implicit operands part of MCID before ones that aren't in addOperand()

[jonpa]

Summary

This addresses the same problem as https://reviews.llvm.org/D46837. Instead of fixing findDefIdx(), this problem can be handled by making addOperand() insert an added operand in a more sensible way.

The reason that this is needed is that a target may lazily change opcode on an instrution with setDesc(), in order to keep all the operands, and then add one extra register operand. If this new register operand is part of the MCInstrDesc (and therefore mapped to a latency in the InstrRW SchedWrites list), it must be inserted in the right place. Therefore, it should be inserted before any operands not part of the MCInstrDesc, but yet found in the MachineInstr operands list. This can typically be an impl-def of a super register.

This is not a guarantee that all operands part of MCInstrDesc end up in the right place, but merely an improvement to let targets do this trick when it's sure it will work out if looking at just the MCInstrDesc and the InstrRW for the instruction.

There are also tests that now fail, which I suspect is due to debug output differences. Before fixing those, we should decide if this patch is the right way to handle this problem.

Not sure if this is reasonable, or if it should instead be demanded by target to handle this on its own (i.e. rebuild the instruction from scratch). This seems however to work well on SystemZ at the moment.

This could possibly be guarded to only be done if the target has a SchedModel.

[MatzeB]

I would consider setDesc() a rare/exceptional operation. So I'm not sure I like an extra loop for the common addOperand() maybe we can rather provide an insertOperand() to accomodate the setDesc() scenario you describe?

[MatzeB]

or maybe prependImplicitDef()?

[jonpa]

This was actually fixed entirely for SystemZ, by just rewriting one setDesc() call to instead rebuilding the new opcode from scratch. So, this is actually not an issue any more of my concern, so I will abandon this for now. Thanks for taking a look, though!

lib/CodeGen/MachineInstr.cpp

void MachineInstr::addOperand(MachineFunction &MF, const MachineOperand &Op) {
   bool isImpReg = Op.isReg() && Op.isImplicit();
   if (!isImpReg && !isInlineAsm()) {
     while (OpNo && Operands[OpNo-1].isReg() && Operands[OpNo-1].isImplicit()) {
       --OpNo;
       assert(!Operands[OpNo].isTied() && "Cannot move tied operands");
     }
   }
 
+  // Let implicit operands (added by e.g. regalloc) that are not part of MCID
+  // remain at the end of the list. This is important for targets with a
+  // SchedModel in the cases where an opcode is changed to a very similar
+  // instruction that also defines e.g. the CC register. To later look up the
+  // latency of this register correctly, this operand must be placed in
+  // accordance with the MCID order of operands.
+  auto isMCIDImplReg = [&] (unsigned Reg) {
+    return (MCID->hasImplicitDefOfPhysReg(Reg) ||
+            MCID->hasImplicitUseOfPhysReg(Reg));
+  };
+  if (isImpReg && isMCIDImplReg(Op.getReg())) {
+        while (OpNo && Operands[OpNo-1].isReg() && Operands[OpNo-1].isImplicit()
+               && !isMCIDImplReg(Operands[OpNo-1].getReg()))
+          --OpNo;
+  }
+
 #ifndef NDEBUG
   bool isMetaDataOp = Op.getType() == MachineOperand::MO_Metadata;
   // OpNo now points as the desired insertion point.  Unless this is a variadic
   // instruction, only implicit regs are allowed beyond MCID->getNumOperands().
   // RegMask operands go between the explicit and implicit operands.
   assert((isImpReg || Op.isRegMask() || MCID->isVariadic() ||
           OpNo < MCID->getNumOperands() || isMetaDataOp) &&
          "Trying to add an operand to a machine instr that is already done!");