[PATCH] D128180: [DebugInfo][InstrRef][NFC] Let LDV handle joins for lists of debug ops

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

This looks good -- I've got some comments, but it's a good separation of the two things going on, selecting the location of an operand, and Doing Stuff (TM) for an entire variable value.

Possibly this is in another patch, but: I think this also deserves some straight-forward unit testing of multiple operands, as far as I can see only the single-operand versions are updated? It would be too much testing to have multiple-operand versions of each flavour of control flow, but some higher level testing, like checking values where one operand has different const-ness and the like don't get joined, should catch potential errors in the future.



================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:2464-2468
-      // For VPHIs where we don't know the location, we definitely can't find
-      // a join loc.
-      if (OutVal.BlockNo != MBB.getNumber())
-        return None;
-
----------------
Deleting this makes me twitch a little; and it looks like the code and comment are out of sync, the test is for whether this is a backedge (VPHI defined in the current block feeding back into itself).

I think the way this would fail is where there's an arbitrary, unresolved VPHI feeding into a block: I think without this test, we would then treat it like a live-through value, even if it ends up being something different. There might be some other portion of code that stops such an error being visible.

Does that make sense / is there a positive reason for deleting this?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:2480
+  for (unsigned Idx = 0; Idx < FirstValue.getLocationOpCount(); ++Idx) {
+    if (!LocOpsToJoin.contains(Idx)) {
+      NewDbgOps.push_back(FirstValue.getDbgOpID(Idx));
----------------
IMO, wants a comment, "If this isn't due to be joined, use the already-agreed value". Seems obvious, but let's affirm future readers understanding.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:2514
+    DbgOpID OutValOpID = OutVal.getDbgOpID(DbgOpIdx);
+    DbgOp OutValOp = DbgOpStore.find(OutValOpID);
+    assert(!OutValOp.IsConst);
----------------
Take reference, avoiding large copy? May or may not have reallocation/invalidation issues.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h:1344-1345
+  /// For the given block and live-outs feeding into it, try to find
+  /// machine locations for every debug operand where all the values feeding
+  /// into that operand join together.
+  /// \returns true if a joined location was found for every value that needed
----------------
Wording nit -- to me this reads better (clear that we're abstracting over each individual operand, and joining all values of an operand together). This is probably pure taste, how do you feel it reads?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h:1353
 
+  Optional<ValueIDNum> pickValuePHILoc(
+      unsigned DbgOpIdx, const MachineBasicBlock &MBB, const LiveIdxT &LiveOuts,
----------------
Could I suggest `pickOperandPHILoc` -- if we're terming each part of the overall Value an operand, we should make the function names consistent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128180