[LiveRangeEdit] Handle instructions with no defs correctly.

[gberry]

Summary

The code in LiveRangeEdit::eliminateDeadDef() that computes isOrigDef
doesn't handle instructions in which operand 0 is not a def (e.g. KILL)
correctly. Add a check that operand 0 is a def before doing the rest of
the isOrigDef computation.

[MatzeB]

Hi Geoff,

testcase?

[gberry]

I ran into this issue when making some other changes, and I'm not sure how to test it in isolation. There are a handful of lit tests that hit this case. For example, if the test added above were instead made an assert(isDef()) inside the body of the if, it would fire. But I don't think there is a case that triggers any testable change in output because of the other conditions checked along with isOrigDef.

For some context, the change I was working on that led to this was adding support for re-materializing COPY XZR on AArch64. When this is done, eliminateDeadDef can be called on a KILL XZR instruction, which leads to an assert in VRM->getOriginal(Dest) on line 277 since XZR isn't a vreg.

I could combine this fix with my change to enable COPY XZR re-materialization if that makes more sense.

[MatzeB]

I'll leave the review to Wei Mi who wrote that part of the code.

Independent of this fix I wonder what happens when there are multiple definitions in an instruction, or implicit definitions which are not at operand position 0? And based on the description of your error symptoms it sounds like a if (TargetRegisterInfo::isVirtualReg()) would be a good idea as well.

[wmi]

Geoff, thanks for the patch. The change looks good to me. I think for now, we don't have to worry about multiple definition because isReallyTriviallyReMaterializableGeneric filter out multidef instructions.

[gberry]

Thanks. I'll add an assert that MI only has one explicit def as well.

lib/CodeGen/LiveRangeEdit.cpp

void LiveRangeEdit::eliminateDeadDef(MachineInstr *MI, ToShrinkSet &ToShrink,
 
   DEBUG(dbgs() << "Deleting dead def " << Idx << '\t' << *MI);
 
   // Collect virtual registers to be erased after MI is gone.
   SmallVector<unsigned, 8> RegsToErase;
   bool ReadsPhysRegs = false;
   bool isOrigDef = false;
   unsigned Dest;
-  if (VRM && MI->getOperand(0).isReg()) {
+  if (VRM && MI->getOperand(0).isReg() && MI->getOperand(0).isDef()) {
     Dest = MI->getOperand(0).getReg();
     unsigned Original = VRM->getOriginal(Dest);
     LiveInterval &OrigLI = LIS.getInterval(Original);
     VNInfo *OrigVNI = OrigLI.getVNInfoAt(Idx);
     // The original live-range may have been shrunk to
     // an empty live-range. It happens when it is dead, but
     // we still keep it around to be able to rematerialize
     // other values that depend on it.