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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

Worth putting implicit-check-not on all the tests?

(got up to loop-unroll.ll)



================
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).
+;;
----------------
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.


================
Comment at: llvm/test/DebugInfo/assignment-tracking/X86/dbg-phi-produces-undef.ll:23
+;;
+;; Check that the dbg.value inserted at XXX is undef because there's no
+;; appropriate alternative value to choose.
----------------
Nit -- something other than literal "XXX" would be preferred, I don't know whether this matches other peoples styles, but I've only ever seen XXX used to indicate TODOs or sketchy situations. In the default vim syntax highlighting config, XXX is highlighted in yellow as a TODO.


================
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)
----------------
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.


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


================
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
----------------
I had the slight expectation that this was going to get a dbg.value attached to it, this is deliberately dropped then?


================
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()
----------------
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.


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

https://reviews.llvm.org/D136335