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

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
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:
> vsk wrote:
> > dblaikie wrote:
> > > 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.
> > I don't understand the concern here. Is it that the current pass behavior (setting a dbg.value operand to undef) preserves semantics, while the proposed new behavior (deleting a dbg.value) doesn't? If so, I don't follow. I'd argue that dbg.value(<alloca>, DW_OP_deref) doesn't mean the same thing as dbg.value(undef, DW_OP_deref), and that the latter is actually ill-formed. (As it happens, check-llvm passes with that criterion added to the verifier -- I'll have to test this out on a larger codebase to see if anything new turns up.)
> Ah, no - my discussion here isn't about the correctness of the production code, but the suitableness of the test.
> 
> Test should be in terms of the contract - which execution of the pass is the one that has done the wrong thing? That and only that one is the one that should be tested, otherwise the other optimization passes may change in ways that cause the test to no longer be applicable. I think it's relatively rare to run more than one optimization pass in a given test ("useful to run a sequence of passes in this case (and this is a widespread practice in llvm)" - that is counter to my general understanding of widespread practice of writing LLVM lit tests)/more common to test only a single optimization pass.
I think this could be resolved by having two tests — one that tests one run of instcombine to define expected input/output, and one "end-to-end" regression test (=this one) that ensures that the intended effect on the pipeline is preserved. Both are valuable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85555