[PATCH] D136335: [Assignment Tracking Analysis][5/*] Tests

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

Orlando added a comment.

(forgot to submit the inline comments)



================
Comment at: llvm/test/DebugInfo/assignment-tracking/X86/dbg-phi-produces-undef.ll:4-5
+
+;; Hand written test because the scenario is unlikely. Check that the "value"
+;; of a debug def PHIs is "undef" (because we don't actually track PHIs).
+;;
----------------
jmorse wrote:
> What would happen if non-constants were used, but there were no out-of-block users? The reason being that today we can recover variable locations that are coincidentally PHI'd correctly due to being in the right registers, even if there isn't an IR PHI happening and being used. But that won't happen if an explicit `DBG_VALUE $noreg` is produced at the merge point.
Interesting question -- the `DBG_VALUE $noreg` is not actually produced at the merge point though, it's inserted after the store which is storing performing assignment named `!2` while the debug program asserts that assignment `!1` should still be visible.

AssignmentTrackingAnalysis just tracks merge points, it does not insert location defs (/undef) at merge points itself.


================
Comment at: llvm/test/DebugInfo/assignment-tracking/X86/diamond-3.ll:33-36
+;; === TODO / WISHLIST ===
+; LEBAL-KCEHC: bb.2.if.end:
+; KCEHC:         CALL64pcrel32 target-flags(x86-plt) @es
+; KCEHC:         DBG_VALUE %stack.0.a.addr, $noreg, ![[A]], !DIExpression(DW_OP_deref)
----------------
jmorse wrote:
> I endorse the syntax; if this isn't happening, isn't that a regression from LowerDbgDeclare? debug-info-on-edges is a serious problem that we don't have a solution for right now sadly.
Yeah it is a regression. I think TODO / WISHLIST should probably be renamed FIXME to indicate this. This is on my mental TODO list but I should add this to the actual TODO list in the documentation - leaving this comment as "not done" as a reminder to do that before landing.


================
Comment at: llvm/test/DebugInfo/assignment-tracking/X86/loop-hoist.ll:9-18
+;; int f(int a, int b, int c) {
+;;   do {
+;;     /* stuff */
+;;     c *= c;
+;;     a = b;
+;;     e();
+;;   } while (d());
----------------
jmorse wrote:
> Just thinking out loud -- I suppose that without assignment tracking, we would end up with one dbg.value in the entry block for the argument, then one in the middle of the `do.body` loop. And because where the `do.body` loop joins there's no PHI to merge those values, there's no way to get a correct variable location on the multiplication.
> 
> I don't see any way to improve this with assignment tracking, right? It always requires there to be a PHI, so that we know which loop iteration we're in.
> 
> (Not a criticism, just confirming to myself that I understand what's going on).
Yeah that all sounds correct.


================
Comment at: llvm/test/DebugInfo/assignment-tracking/X86/loop-sink.ll:57
+while.end:                                        ; preds = %while.body, %entry
+  %storemerge.lcssa = phi i32 [ %a, %entry ], [ %b, %while.body ]
+  store i32 %storemerge.lcssa, ptr %a.addr, align 4, !DIAssignID !20
----------------
jmorse wrote:
> I had the slight expectation that this was going to get a dbg.value attached to it, this is deliberately dropped then?
As in by the analysis pass (as a DBG_VALUE/INSTR_REF), or in this IR? Wasn't there a patch that stopped `insertDebugValuesForPHIs` being called by LCSSA maybe a year or so ago?


================
Comment at: llvm/test/DebugInfo/assignment-tracking/X86/loop-sink.ll:29-30
+; CHECK-LABEL: bb.2.while.body:
+;; It's unfortunate that we see $noreg here -- the vreg use gets lost at some
+;; point by SelectionDAG.
+; CHECK: DBG_VALUE $noreg, $noreg, ![[A]], !DIExpression()
----------------
jmorse wrote:
> StephenTozer wrote:
> > Could you try and test this with a vreg that isn't dropped? Checking for `$noreg` means that the expected behaviour is only being half tested for imo; if this particular "use a DbgDef instead of a MemDef" branch had a bug and just dropped defs instead of using the correct `Value`, this test wouldn't pick it up.
> Looks like it's because it's an Argument rather than any other value -- I agree that it'd be better to fiddle with the test to avoid checking for undef, this will ensure good coverage in the future.
Sorry I left this one not-done to come back to, and forgot to come back to it. Done.


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

https://reviews.llvm.org/D136335