[PATCH] D128211: [DebugInfo][InstrRef][NFC] Handle transfers of variadic debug values in InstrRefLDV

Mon Aug 22 00:29:05 PDT 2022

StephenTozer added inline comments.

================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:819-820
     auto MovingVars = ActiveMLocs[Src];
-    ActiveMLocs[Dst] = MovingVars;
+    for (auto Var : MovingVars)
+      ActiveMLocs[Dst].insert(Var);
     VarLocs[Dst.asU64()] = VarLocs[Src.asU64()];
----------------
Orlando wrote:
> StephenTozer wrote:
> > jmorse wrote:
> > > Insert the range perhaps? Also, isn't this a meaningful change from the prior implementation -- variables that were active at the destination used to be erased (in the overwrite), now they'll remain active. I can't remember whether this presents an issue, but is it necessary to change the behaviour?
> > I'll double check this one. To summarize: we assume the destination to have been pre-clobbered. The reason we do this instead of clobbering it here is that when there is an "identity" copy, we may 1) clobber the location `Dst`, 2) select a backup location for those values in `Src`, and 3) subsequently declare those variables to be live in `Dst`. This change to the logic happened in D128101; I'm not sure if this is an unnecessary change on top of that one, but I'm moderately sure I came across //some// case where it was an issue; I'll double check and get a test case out this time.
> Did you check this?
Checked, the reason we need this change is because as of the prior changes described above, if we copy from $r10 to $r11 when both already contain the same value, when we "clobber" $r11 it is possible the variables that were live there will continue to be live there. Then, if we choose to transfer the variables that were live in $r10 to be live in $r11, we do not want to terminate the perfectly valid variables that are live in $r11.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128211