This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo][InstrRef] Fix error in copy handling in InstrRefLDV
ClosedPublic

Authored by StephenTozer on Jun 17 2022, 3:00 PM.

Details

Summary

Currently, an error exists when InstrRefBasedLDV observes transfers of variables across copies, which causes it to lose track of variables under certain circumstances, resulting in shorter lifetimes for those variables as LDV gives up searching for live locations for them.

The issue can be summarised as, when the TransferTracker observes a copy, under certain circumstances it may start tracking variables, that previously were in the source location, in the destination location instead. This is obviously valid, but it then tries to clobber the destination location due to it being overwritten, including clobbering the variables that were just moved to use this location. This does not immediately kill the variable, but causes it to stop being tracked, potentially ending its lifetime early.

This patch fixes this issue by first tracking the specific set of variables and values that already live in the destination, then after updating our underlying store of values (in the MLocTracker) we try to find new locations for all those variables that have been clobbered. Only after that do we potentially move other variables into the destination location, ensuring that no variables are erroneously clobbered and all the existing variables in the destination are given proper homes.

Diff Detail

Event Timeline

StephenTozer created this revision.Jun 17 2022, 3:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2022, 3:00 PM
StephenTozer requested review of this revision.Jun 17 2022, 3:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2022, 3:00 PM
Orlando added inline comments.Jul 7 2022, 7:08 AM
llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
1777

Is it significant that the "The copy might have clobbered variables based on the destination register" loop has moved above up above this part?

llvm/test/DebugInfo/X86/instr-ref-track-clobbers.mir
16–17

Is the unordered-ness of CHECK-DAG limited to anything between the previous and next CHECK lines or not limited at all, do you know?

Personally I think it would be better to simplify the test to check for the behaviour that occurs now with a comment explaining other permissible behaviour, rather than trying to encode all correct behaviours. wdyt?

StephenTozer added inline comments.Jul 7 2022, 8:16 AM
llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
1777

Yes - the order in which we want these operations performed is:

  1. Keep track of the variables in the Dest and what value(s) they referred to.
  2. Update our location->value tracker to reflect the copy.
  3. Find a new location for (or undef) the variables that were in Dest.
  4. 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.

StephenTozer added inline comments.Jul 8 2022, 5:13 AM
llvm/test/DebugInfo/X86/instr-ref-track-clobbers.mir
16–17

I believe the CHECK-DAG lines will still be checked between the previous and next non-DAG directive. I'm personally ambivalent about the "check current behaviour with a comment" vs "check any permissible behaviour" approaches, happy to switch to the former!

Orlando added inline comments.Jul 8 2022, 7:49 AM
llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
1777

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)?

llvm/test/DebugInfo/X86/instr-ref-track-clobbers.mir
16–17

I would prefer that - it'd be easier to read and there will be less room for things to slip through unnoticed.

StephenTozer added inline comments.Jul 8 2022, 9:00 AM
llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
1777

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.

Orlando accepted this revision.Jul 8 2022, 9:05 AM

LGTM (once the test is updated), thanks.

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
1777

Ok great - thanks for the explanation! With that, I'm happy to approve.

This revision is now accepted and ready to land.Jul 8 2022, 9:05 AM
This revision was landed with ongoing or failed builds.Jul 11 2022, 5:39 AM
This revision was automatically updated to reflect the committed changes.