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

Mon Sep 28 17:12:49 PDT 2020

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:
> vsk wrote:
> > dblaikie wrote:
> > > 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?
> > dbg.value can be used to indicate the value inside of an alloca starting at a specific position in the instruction stream, and continuing until the the dbg.value is superseded (e.g. by a dbg.value intrinsic describing another load/store from/to the alloca, or by the register holding the loaded value being clobbered).
> > 
> > When a variable backed by an alloca stops being described by a dbg.declare, and transitions to being described by a series of dbg.value intrinsics, a store to the alloca would be unrelated to any dbg.value derived from the alloca. A problem with this approach is that unless specific effort is made to insert a dbg.value after each store-like instruction which modifies the alloca, the debug info becomes less precise.
> > 
> > There's a FIXME describing this problem above the comment describing ShouldLowerDbgDeclare. I don't think we can get away from LowerDbgDeclare in general, but we could push towards using it only when we know an alloca is to be deleted (i.e. do it lazily, instead of eagerly like we do today). I'm not sure whether this patch would still be necessary if LowerDbgDeclare were lazy, but will look into this.
> I looked into deferring LowerDbgDeclare to the points where llvm actually needs to erase an alloca, but found that doing so might not be practical. The problem is that llvm can delete stores into an alloca without deleting the alloca itself. Without lowering dbg.declares eagerly, we may lose track of updates to a variable that don't get stored back into an alloca. So I believe some version of this patch is necessary.
I think I follow now - though I think it'd still be beneficial to describe this in terms of the IR contract rather than with reference to other passes in the pipeline. "Given this IR, doing X would produce this erroneous result (the IR would now describe the value of a variable as having some value it never had in the input IR, for instance)".

In any case, I'm likely not the best person to review the code change here, not being as familiar with debug variable location tracking - happy to bow out/leave it up to others even if they disagree with me on this point (though hope they take it under consideration). The test point I made elsewhere I still feel pretty strongly about.

================
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:
> > 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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85555