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

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando 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());
----------------
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)?


================
Comment at: llvm/test/DebugInfo/X86/instr-ref-track-clobbers.mir:16-17
+## after the move to $r15 or immediately after $rax is clobbered, but no later.
+# CHECK-DAG: DBG_VALUE $r15,
+# CHECK-DAG: $rax = LEA64r
+# CHECK: $rax = MOV64rr
----------------
StephenTozer wrote:
> Orlando wrote:
> > 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?
> 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!
I would prefer that - it'd be easier to read and there will be less room for things to slip through unnoticed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128101