Page MenuHomePhabricator

[LiveRangeEdit] Handle instructions with no defs correctly.
ClosedPublic

Authored by gberry on Nov 28 2016, 12:08 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

gberry updated this revision to Diff 79432.Nov 28 2016, 12:08 PM
gberry retitled this revision from to [LiveRangeEdit] Handle instructions with no defs correctly..
gberry updated this object.
gberry added reviewers: qcolombet, MatzeB, wmi.
gberry added a subscriber: llvm-commits.
MatzeB edited edge metadata.Nov 28 2016, 12:13 PM

Hi Geoff,

testcase?

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.

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 accepted this revision.Nov 28 2016, 2:36 PM
wmi edited edge metadata.

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.

This revision is now accepted and ready to land.Nov 28 2016, 2:36 PM

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

This revision was automatically updated to reflect the committed changes.