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

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

Orlando accepted this revision.
Orlando added a comment.
This revision is now accepted and ready to land.

> loadInLocs is getting pretty big now -- if there's an obvious portion that can be split into a method, or a helper lambda that can be defined with less indentation, that'd help.

Have you addressed this?

> Further comment: this patch doesn't have tests currently as there are no existing unit tests for transfer tracking, and there are no changes that would be visible to an MIR test. I could create a suite of transfer tracking unit tests as part of this patch, though if possible I'd prefer that not to block this patch, since it would probably involve writing tests for more behaviour than is featured in this patch alone.

It would be reassuring to demonstrate that the feature as a whole is working with this patch in. A few llvm-lit tests for this purpose may be beneficial, or if such tests exist in other patches, mentioning them in the patch summary and in the commit message would be useful IMO.

LGTM once these comments have been addressed and on the understanding that lit tests for the not-yet-testable variadic use-before-defs are added when variadic `DBG_INSTR_REF` support is added.



================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:428
+                       const DbgValueProperties &Properties,
+                       SmallVectorImpl<DbgOp> &DbgOps, unsigned Inst) {
+    UseBeforeDefs[Inst].emplace_back(DbgOps, Var, Properties);
----------------
Can `DbgOps` be a `const &`?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:775
+        for (LocIdx Loc : ActiveVLocIt->second.loc_indices()) {
+          // We'
+          if (Loc != MLoc)
----------------
Incomplete comment here


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128211