[PATCH] D128101: [DebugInfo][InstrRef] Fix error in copy handling in InstrRefLDV

[Stephen Tozer via Phabricator via llvm-commits]

StephenTozer added inline comments.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:1777
   if (TTracker && isCalleeSavedReg(DestReg) && SrcRegOp->isKill())
     TTracker->transferMlocs(MTracker->getRegMLoc(SrcReg),
                             MTracker->getRegMLoc(DestReg), MI.getIterator());
----------------
Orlando wrote:
> StephenTozer wrote:
> > Orlando wrote:
> > > Is it significant that the "The copy might have clobbered variables based on the destination register" loop has moved above up above this part? 
> > Yes - the order in which we want these operations performed is:
> > 
> >   # Keep track of the variables in the Dest and what value(s) they referred to.
> >   # Update our location->value tracker to reflect the copy.
> >   # Find a new location for (or undef) the variables that were in Dest.
> >   # Now that "Dest" is empty, transfer the variables that were in Source to Dest.
> > 
> > Step 3 should come before step 4 because we want to empty out the bucket for Dest before moving other variables into it, otherwise TTracker may be confused about which variables were previously there and which ones have just been moved there.
> > 
> > 
> That makes sense, thanks. One last thing then - is it necessary for 2 (`performCopy`) to come between 1 and 3, or could we fold 3 into 1 (i.e. do the `clobberMloc` call in the first loop rather than separately)?
3 should come after 2, because the behaviour of step 3 is somewhat dependent on step 2; step 2 updates the MLocTracker (which maps locations -> values), and step 3 then uses that updated location->value mapping. This is necessary to make sure that we don't assume that the current location is automatically a valid location (since it would still appear to contain the right value at that point), while at the same time being able to choose the current location if we have an identity copy (silly, but it can actually happen).

Technically we could try and account for this in step 3 and fold it into 1, but I think that actually muddles the logic, since it would mean the TransferTracker has to start assuming that maybe the MLocTracker has wrong info and trying to account for that, while taking this approach means that the TTracker doesn't have to worry about that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128101/new/

https://reviews.llvm.org/D128101