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

[Stephen Tozer via Phabricator via llvm-commits]

StephenTozer added a comment.

Changes all LGTM, except the one unaddressed comment about not using `$noreg` in `loop-sink.ll`.



================
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)
----------------
Orlando wrote:
> 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). 
Sure - at least it wouldn't be a "silent" failure, and if someone is modifying the redundant debug values code it shouldn't be hard to identify the cause of this failure. Though it also wouldn't hurt for posterity to add a comment along the lines of "TODO: If removeRedundantDbgInstrs gets smarter, add an instruction here to preserve this set of debug values".


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

https://reviews.llvm.org/D136335