[PATCH] D85555: [InstCombine] Remove dbg.values describing contents of dead allocas

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:2655
+    //     dbg.value(i32* %a, "arg0", DW_OP_deref)
+    //     call void @trivially_inlinable_no_op(i32* %a)
+    //     ret void
----------------
dblaikie wrote:
> What if there was a later store to this alloca? Wouldn't removing the dbg.value with deref then let the %0 location persist, passing through the store (without reflecting that store in the dbg.value) - changing the values described in the DWARF, possibly incorrectly?
If there was a store to the alloca after the call, InstCombine must have inserted a dbg.value after the store during LowerDbgDeclare. Removing the dbg.value(..., DW_OP_deref) before the call would extend the range of the initial %0 location until the dbg.value for the store.


================
Comment at: llvm/test/Transforms/InstCombine/erase-dbg-values-at-dead-alloc-site.ll:6-9
+;   - The first InstCombine run converted dbg.declares to dbg.values using the
+;     LowerDbgDeclare utility. This produced a dbg.value(i32* %2, DW_OP_deref)
+;     (this happens when the contents of an alloca are passed by-value), and a
+;     dbg.value(i32 %0) (due to the store of %0 into the alloca).
----------------
dblaikie wrote:
> I don't think the test case should be running instcombine twice, nor should it be running the inliner - it should only be testing the specific behavior of "given IR1, this pass should produce IR2".
> 
> The context can be helpful in comments - but tests/design should be around "this is or is not a valid/good quality optimization, no matter where the IR comes from".
The bug this patch tries to address fundamentally involves a sequence of optimizations. I think it's useful to run a sequence of passes in this case (and this is a widespread practice in llvm), since it gives a fuller picture of what problem is being addressed. I do also think a more targeted/standalone example that just runs instcombine once is useful (this is '@t1' in the test).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85555