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

[David Blaikie via Phabricator via llvm-commits]

dblaikie 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
----------------
vsk wrote:
> 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.
Rather than what InstCombine would've done - I'm interested in what the IR semantics/contract are.

I guess dbg.value only would be used to represent the load from the alloca, not the value in the alloca itself? So a future store to the alloca is unrelated to any dbg.value derived from (what must be loads) from the alloca?


================
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).
----------------
vsk wrote:
> 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).
IR has a defined semantic - I think we should be thinking about passes in terms of whether they preserve the semantics of the IR or not - regardless of where the input IR comes from/what form it's in.

I assume tests that test a sequence of transformations are the minority in LLVM? (in the same way that we don't do end-to-end testing from Clang through IR when we can help it - and in middle end optimizations we generally can test a single pass in isolation & mostly do it that way, don't we?)

Except for analyses, which don't have a serialization, so we have to rely on testing both the analysis & either testing its dump output, or testing how the analysis behavior influences some transformation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85555