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

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

Orlando added a comment.

Thanks for another eagle-eyed review @StephenTozer



================
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
----------------
StephenTozer wrote:
> 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?
>  as far as I can tell continuing to use a stack location for the [0, 64] fragment would be correct in this case

That's right. We don't need to redeclare that the `[0, 64)` fragment is on the stack because the `[64, 128)`  fragment loc def doesn't overlap the previous loc def for `[0, 64)`, and non-overlapping fragment locations compose properly (i.e., mem-loc-frag-fill doesn't need to do anything here because it already works as expected). 


================
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)
----------------
StephenTozer wrote:
> 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.
>  f the "remove redundant debug values" step understands that the first DBG_VALUE is entirely covered by the following two fragments

IMO it'd be better to update the test if/when that happens - it feels like slightly unnecessary future-proofing to me. wdyt?

The important parts of the test are steps 4 (value/value) and 5 (mem/value after untagged store to lower bits). 


================
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.
----------------
StephenTozer wrote:
> 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?
Yeah that's right


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

https://reviews.llvm.org/D136335