[PATCH] D125953: [DebugInfo][InstrRef] Handle joins PHI+Def values in LiveDebugValues

[Jeremy Morse via Phabricator via llvm-commits]

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

LGTM, with some quibbles.



================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:2504-2505
 
+    if (V.second->ID != ValueIDNum::EmptyValue && V.second->ID == FirstVal.ID)
+      continue; // No disagreement.
+
----------------
jmorse wrote:
> IMO this needs a more precise (but still short comment), indicating _what_ about the agreement is acceptable here. i.e., "The difference is only that one value comes from a DBG_PHI, the other from a defining instruction or similar", or something in that vein.
I'm a terrible person for wanting to copy-edit your comments; but could you explicitly mention VPHI / Def conflict, i.e. "such as the same value from different sources (VPHI / Def)".

The reason is that I'm confident that I or some future developer won't be able to work out what's going on unless it's made extremely obvious. It's also why adding the unit test for this is awesome!


================
Comment at: llvm/test/DebugInfo/X86/instr-ref-join-vlocs.ll:52-53
+if.then.i.i.i626:                                 ; preds = %if.then.i620
+  %call.i.i.i.i655 = invoke i8* undef(i64 0, i32 0)
+          to label %allocate.exit.i.i630 unwind label %lpad13
+
----------------
I believe there's an active request (example: https://reviews.llvm.org/D127492#3577259 ) to not put undef into new tests :(. Can this be replaced with poison?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125953