[MachineScheduler] Skip an implicit def of a super-reg added by regalloc in findDefIdx.

[jonpa]

Summary

When working with the scheduler in regards to SystemZ WriteLatencys, I came across cases where the mapping from SchedWrite entry to MI def operands (mapped by the ordering) were corrupted by implicit def operands added by regalloc. The specific register was "CC", which is an implicitly defined register part of the MCInstrDesc.

Since regalloc will not put its implicit-def operands last in list, I believe the right fix is to ignore those when carefully looking up the def index in findDefIdx.

If approved, I will hopefully commit this soon with SystemZ SchedModel improvements.

[atrick]

That's interesting. @MatzeB is this the right fix? Is regalloc supposed to be adding implicit defs in front of the uses, and don't we/shouldn't we have a general API to handle this?

[MatzeB]

(FYI: I'm busy with non-llvm at the moment, so I may not be responsive for things I cannot answer right away; but I'll try to find a timeslot this week going through the backlog of llvm things I have to look at)

[jonpa]

ping.

[materi]

lib/CodeGen/TargetSchedule.cpp

158

"We need to skip..." looks strange to me.

I would prefer if an implicit operand means the same thing regardless of whether it's in the Tablegen file or not.

Maybe it's the adding of the implicit operand that you need to inhibit in your case?

[jonpa]

lib/CodeGen/TargetSchedule.cpp

158

I agree that this is all a bit strange to have to handle this way. This however not a special case, but as far as I know the long since behavior of the register allocator generally.

Since you can't write a scheduler description (where you have to list the def-operand latencies for each def-operand in the right order), based on what regalloc may or may not do, they can't be treated the same.

It seems weird that these regalloc operands end up in the middle of the operand list, but not sure if that's something we could fix now. This patch at least fixes the DAG operand latencies for now...

[jonpa]

ping!

I suspect handling the way MachineOperands are added to the MachineInstr is not going to be simple, so I am hoping we can fix findDefIdx() with this patch for now?

[jonpa]

It seems that the reason this problem occurs for me, is that the target is lazily just calling setDesc() on an instruction to change the opcode, and then adds one extra register. For example, a load may become load-and-test, which just means that the opcode changes and the CC register is defined also.

An alternative way of handling this in MachineInstr::addOperand() is now at: https://reviews.llvm.org/D47524.

It seems to me that we could either:

1. Fix findDefIdx per this patch. The mapping of SchedWrite order to operand order is already in need of a separate lookup, so this might not be so bad.
2. Fix addOperand() per D47524, so that target is free to think about MCInstrDesc and InstrRW for the instruction without interference from super-reg implicit defs etc.
3. Demand of target that it has to build the instruction with the right operand order. This is a bit of extra work for the target: it has to rebuild from scratch and also transfer any non-MCInstrDesc operands to the new instruction.

[jonpa]

r334161 fixes this issue for SystemZ by fixing the backend per alternative (3) above.

Is there any interest for (1) or (2), or should I abandon them?

[javed.absar]

In D46837#1124532, @jonpa wrote:

r334161 fixes this issue for SystemZ by fixing the backend per alternative (3) above.

Is there any interest for (1) or (2), or should I abandon them?

Option (2) seems a decent practical solution. Would a lot of cases fail because of it, and would existing sched-models pick wrong latencies in some cases?

[jonpa]

Abandoning this for now at least. It seems good enough to simply make it the rule to rebuild MIs from scratch instead of using setDesc() in cases where the MCInstrDesc has an implicit operand that should go before the RA operands...

lib/CodeGen/TargetSchedule.cpp

#endif
   }
   return SCDesc;
 }
 
 /// Find the def index of this operand. This index maps to the machine model and
 /// is independent of use operands. Def operands may be reordered with uses or
 /// merged with uses without affecting the def index (e.g. before/after
 /// regalloc). However, an instruction's def operands must never be reordered
-/// with respect to each other.
+/// with respect to each other.  We need to skip implicit def operands (added
+/// by regalloc) that are not part of the MCInstrDesc.
 static unsigned findDefIdx(const MachineInstr *MI, unsigned DefOperIdx) {
+  const MCInstrDesc &MID = MI->getDesc();
   unsigned DefIdx = 0;
   for (unsigned i = 0; i != DefOperIdx; ++i) {
     const MachineOperand &MO = MI->getOperand(i);
-    if (MO.isReg() && MO.isDef())
+    if (MO.isReg() && MO.isDef() &&
+        (!MO.isImplicit() || MID.hasImplicitDefOfPhysReg(MO.getReg())))
       ++DefIdx;
   }
   return DefIdx;
 }
 
 /// Find the use index of this operand. This is independent of the instruction's
 /// def operands.
 ///