Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

Insert IMPLICIT_DEFS for undef uses in tail merging

Authored by kparzysz on Aug 7 2017, 10:21 AM.



Tail merging can convert an undef use into a normal one when creating a common tail. Doing so can make the register live out from a block which previously contained the undef use. To keep the liveness up-to-date, insert IMPLICIT_DEFs in such blocks when necessary.

Diff Detail


Event Timeline

kparzysz created this revision.Aug 7 2017, 10:21 AM
MatzeB added inline comments.Aug 7 2017, 10:40 AM

I'm confused: Don't you need to compare the live-outs of Pred with the live-ins of NewDest? Adding the liveouts of Pred and stepping backwards over the instructions gives you the live-ins of Pred, but I don't see what they are good for.

I would have expected this to look like this:

// Add implicit defs for NewDest live-in values not available at end of Pred.
for (const MachineBasicBlock::RegisterMaskPair &LI : NewDest->liveins()) {
  // computeLiveIn() currently doesn't add partial live-ins so we are fine with an assert.
  assert(LI.LaneMask == LaneBitmask::getAll() && "found partial live-in");
  if (PredLiveOuts.available(*MRI, LI.PhysReg)) {
    BuildMI ...
kparzysz added inline comments.Aug 7 2017, 11:02 AM

You're right, this won't work. Still, we need to traverse the block, and it will have to use stepForward. Consider this case:

  liveins: R0
  R0<def,dead> = ... R0<kill>    ; stays in MBB
  ... = R0<undef>                ; goes into common tail, but no longer <undef>

We need to add R0<def> = IMPLICIT_DEF here.

We can't rely on addLiveOuts from LivePhysRegs, because NewDest->liveins() will (by definition) be a subset of Pred->addLiveOuts.

kparzysz updated this revision to Diff 110043.Aug 7 2017, 12:06 PM

Changed how the liveness inconsistencies are detected. Added comments with explanations.

kparzysz updated this revision to Diff 110044.Aug 7 2017, 12:08 PM

Remove an accidental change in BranchFolding.h.

MatzeB added inline comments.Aug 22 2017, 2:16 PM

We can't rely on addLiveOuts from LivePhysRegs, because NewDest->liveins() will (by definition) be a subset of Pred->addLiveOuts.

Fair point. This is trickier than it seemed at first. I'm still convinced though that it can be done, and I really want to avoid stepForward(). I'll have a shot at implementing it.

MatzeB edited edge metadata.Aug 22 2017, 4:00 PM

This should fix the problem without using stepForward:

kparzysz abandoned this revision.Sep 7 2017, 7:48 AM

Replaced by D37034.