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

Fri Nov 4 06:32:28 PDT 2022

StephenTozer added a comment.

Some minor comments, but otherwise LGTM.

================
Comment at: llvm/test/DebugInfo/assignment-tracking/X86/diamond-1.ll:28
+;;   a = 500
+;; if.then:
+;;   store (phi if.then: 100, if.else: 500)
----------------

================
Comment at: llvm/test/DebugInfo/assignment-tracking/X86/diamond-1.ll:46
+; CHECK-NEXT: MOV32mr %stack.0.a.addr, 1, $noreg, 0, $noreg, %0
+; CHECK-NEXT: DBG_VALUE %stack.0.a.addr, $noreg, !12, !DIExpression(DW_OP_deref)
+
----------------

================
Comment at: llvm/test/DebugInfo/assignment-tracking/X86/diamond-2.ll:16
+;;   a = 500
+;; if.then:
+;;   store (phi if.then: 100, if.else: 500)
----------------

================
Comment at: llvm/test/DebugInfo/assignment-tracking/X86/diamond-2.ll:34
+; CHECK-NEXT: MOV32mr %stack.0.a.addr, 1, $noreg, 0, $noreg, %0
+; CHECK-NEXT: DBG_VALUE %stack.0.a.addr, $noreg, !12, !DIExpression(DW_OP_deref)
+
----------------

================
Comment at: llvm/test/DebugInfo/assignment-tracking/X86/diamond-3.ll:19-21
+;;    ; memory location at call sites leaking the address (in an idea world,
+;;    ; the memory location always be in use at that point and so this wouldn't
+;;    ; be necessary).
----------------

================
Comment at: llvm/test/DebugInfo/assignment-tracking/X86/loop-hoist.ll:20
+
+;; The variable of interst is `a`, which has a store that is hoisted out of the
+;; loop into the entry BB. Check the memory location is not used after the
----------------

================
Comment at: llvm/test/DebugInfo/assignment-tracking/X86/loop-hoist.ll:33
+
+; CHECK: bb.1.do.body:
+; CHECK:      DBG_INSTR_REF 2, 0, ![[C]], !DIExpression()
----------------
Are the variables other than "a" actually needed for this test? Usually I think we'd delete variables that aren't relevant to the behaviour being tested.

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

================
Comment at: llvm/test/DebugInfo/assignment-tracking/X86/loop-sink.ll:42
+; Function Attrs: mustprogress uwtable
+define dso_local noundef i32 @_Z1fii(i32 noundef %a, i32 noundef %b) local_unnamed_addr #0 !dbg !12 {
+entry:
----------------
Same comment as the above test, if the variables other than "a" aren't being tested for could they be deleted from the test?

================
Comment at: llvm/test/DebugInfo/assignment-tracking/X86/lower-to-value.ll:8
+; RUN:    -experimental-debug-variable-locations=true \
+; RUN: | FileCheck %s --check-prefixes=CHECK,INSTRREF --implicit-check-not=DBG_VALUE
+
----------------
Add an implicit-check-not for `DBG_INSTR_REF` for the experiemental-debug-locations case?

================
Comment at: llvm/test/DebugInfo/assignment-tracking/X86/lower-to-value.ll:46-47
+
+;; The final assignment (X.B += 2) doesn't get stored back to the alloca. This
+;; means that that the stack location isn't valid for the entire lifetime of X.
+; DBGVALUE: %2:gr64 = nsw ADD64ri8 %1, 2, implicit-def dead $eflags, debug-location
----------------
More just a question for my understanding than anything relevant to the test, but as far as I can tell continuing to use a stack location for the [0, 64] fragment would be correct in this case - is a partial invalidation of the stack location pessimistically considered to invalidate the whole stack location in some cases, and if so what makes it different from the frag-agg case (where we do redeclare the stack location for the valid bytes) in the next test?

================
Comment at: llvm/test/DebugInfo/assignment-tracking/X86/mem-loc-frag-fill-cfg.ll:11
+;; Check that the frag-agg pseudo-pass works on a simple CFG. When LLVM sees a
+;; dbg.value with an overlapping fragment it essentially consideres the
+;; previous location as valid for all bits in that fragment. The pass inserts
----------------

================
Comment at: llvm/test/DebugInfo/assignment-tracking/X86/mem-loc-frag-fill-cfg.ll:47
+;; Most check lines are inline in main.
+; CHECK-DAG: ![[nums:[0-9]+]] = !DILocalVariable(name: "nums",
+
----------------
Nit, but I don't think this needs to be a `CHECK-DAG`, the metadata section comes before the MIR contents of the file iirc.

================
Comment at: llvm/test/DebugInfo/assignment-tracking/X86/mem-loc-frag-fill.ll:8
+; RUN:    -experimental-debug-variable-locations=true \
+; RUN: | FileCheck %s  --implicit-check-not=DBG_VALUE
+
----------------

================
Comment at: llvm/test/DebugInfo/assignment-tracking/X86/mem-loc-frag-fill.ll:31
+;; Most check lines are inline in main.
+; CHECK-DAG: ![[nums:[0-9]+]] = !DILocalVariable(name: "nums",
+
----------------
Ditto above, no need for -DAG

================
Comment at: llvm/test/DebugInfo/assignment-tracking/X86/no-redundant-def-after-alloca.ll:3
+; RUN:    -experimental-assignment-tracking  \
+; RUN: | FileCheck %s --implicit-check-not=DBG_VALUE
+
----------------

================
Comment at: llvm/test/DebugInfo/assignment-tracking/X86/no-redundant-def-after-alloca.ll:5
+
+;; Hand written. Check that no unecessary undef is inserted after an alloca
+;; that has a linked dbg.assign that doesn't immediately follow it.
----------------

================
Comment at: llvm/test/DebugInfo/assignment-tracking/X86/untagged-store-frag.ll:9
+;; 2. dbg(a): bits [0,  64) = !14 ; Use memory loc
+;; 3. dbg(a): bits [0,  32) = <unique ID> ; dbg.value has no ID
+;; 4. dbg(a): bits [32, 64) = !16 ; These bits don't use mem loc.
----------------
Just for formatting consistency, maybe include the "Use X loc" here.

================
Comment at: llvm/test/DebugInfo/assignment-tracking/X86/untagged-store-frag.ll:13
+;; ...                                                                ; |
+;; 5. mem(a): bits [0,  32) = <unique ID> ; Untagged                  ; |
+;; ..                                                                 ; |
----------------

================
Comment at: llvm/test/DebugInfo/assignment-tracking/X86/untagged-store-frag.ll:29-31
+; CHECK:      DBG_VALUE %stack.0.a.addr, $noreg, ![[A]], !DIExpression(DW_OP_deref)
+; CHECK-NEXT: DBG_VALUE 5,               $noreg, ![[A]], !DIExpression(DW_OP_LLVM_fragment, 0, 32)
+; CHECK-NEXT: DBG_VALUE $noreg,          $noreg, ![[A]], !DIExpression(DW_OP_LLVM_fragment, 32, 32)
----------------
With these adjacent `DBG_VALUE`s describing the same variable, if the "remove redundant debug values" step understands that the first DBG_VALUE is entirely covered by the following two fragments then this test would start to fail. Maybe add an additional instruction between the first `dbg.assign` and `dbg.value` so that these two are not redundant (even if they currently aren't recognized as being so)? AFAIK that might also result in an extra debug value being produced by the agg-frag step, but I don't think there's any problem with that being included in the test if so.

================
Comment at: llvm/test/DebugInfo/assignment-tracking/X86/use-known-value-at-early-mem-def-2.ll:5
+
+;; Check that sandwitching instructions between a linked store and dbg.assign
+;; results in a dbg.value(prev_value) being inserted at the store, and a
----------------

================
Comment at: llvm/test/DebugInfo/assignment-tracking/X86/use-known-value-at-early-mem-def-2.ll:43-45
+  ;; the fragment is "unknown". TODO: In this case the value of the fragment is
+  ;; still obviously 5; a future improvement could be to be smarter and work
+  ;; this out. But that's a lot of work for an uncommon case.
----------------
Just to confirm - we're aware that `c = 5`, but because we're trying to produce an implicit value for the fragment it would take a bit of work to confirm what the lower 32 bits actually are? Which I think (assuming we don't examine the constant itself - also this is not a request to implement as part of this patch stack) would require us to insert an AND into the DIExpression to pull out only the lower 32 bits of the implicit value, i.e. `DBG_VALUE 5, $noreg, ![[var]], !DIExpression(DW_OP_constu 0xffffffff, DW_OP_and, DW_OP_stack_value)`. Does that sound correct?

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

https://reviews.llvm.org/D136335