[PATCH] D48331: [DebugInfo][InstCombine] Preserve DI after combining zext instructions

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: test/Transforms/InstCombine/debuginfo-trunc-and-zext.ll:1
+; ModuleID = 'debuginfo-trunc-and-zext.ll'
+source_filename = "debuginfo-trunc-and-zext.ll"
----------------
vsk wrote:
> bjope wrote:
> > bjope wrote:
> > > vsk wrote:
> > > > bjope wrote:
> > > > > vsk wrote:
> > > > > > lebedev.ri wrote:
> > > > > > > This does not actually check anything, there is no `RUN` line.
> > > > > > Right. When you add a run line, please reduce this test by invoking `opt -debugify`, instead of checking in unnecessary metadata lines.
> > > > > Is that really a good idea?
> > > > > 1) It is really hard to review this without knowing what the input looks like.
> > > > > 2) As this will be a regression test, isn't it better to have the input fixated. Who know what -debugify will generate for this input in the future.
> > > > > 3) There might be lots of redundant debug info generated by -debugify, making it hard to understand the test case if it fails in the future (specially without knowing if -debugify generated something differently originally).
> > > > The basic requirement of -debugify output is that it must be stable enough to use in regression tests. Just to recap, a dbg.value is inserted after each non-void-valued instruction if the insertion point is valid, and the variable names are sequential integers. Occasionally -debugify requires bug fixes, but these changes aren't allowed to break existing tests or to change their meaning. I hope that addresses your first and second concerns? It does take some extra effort initially to understand this style of test, but I think that effort pays off.
> > > > 
> > > > Concretely, the advantage of using -debugify in regression tests is that it lets us test an optimization in the exact same place as the optimization's ability to preserve debug info. This makes it easier to keep debug info tests in sync with the behavior of an optimization. And it makes it much easier to add tests by lessening the need for manual reduction, which is a barrier to contribution.
> > > > 
> > > > To your third point, you're right that -debugify inserts more debug info than strictly needed, but we can avoid confusion here by using strict CHECK lines.
> > > So the size of the variable always matches the value exactly?
> > > And it will never happen that -debugify will simulate that SROA has splitted an aggregate into several smaller pieces? 
> > > And we will never let -debugify generate DIExpressions?
> > > 
> > > Anyway, my main concern is that it will be really hard to review patches like this one. without knowing what the input looks like. But maybe that is obvious after playing around with debugify for some hours.
> > > Concretely, the advantage of using -debugify in regression tests is that it lets us test an optimization in the exact same place as the optimization's ability to preserve debug info.
> > 
> > I guess there is a test case for the optimization somewhere, is the idea that this test shouuld be added as an extra RUN line in that test case instead of adding a new file?
> > 
> > 
> > So the size of the variable always matches the value exactly?
> 
> Yes, specifically, DataLayout::getTypeAllocSizeInBits is used for sized types.
> 
> > And it will never happen that -debugify will simulate that SROA has splitted an aggregate into several smaller pieces?
> > And we will never let -debugify generate DIExpressions?
> 
> I view these specific changes as a bit out of scope for -debugify, but regardless, it's possible to write regression tests which don't depend on these details.
> 
> > Anyway, my main concern is that it will be really hard to review patches like this one. without knowing what the input looks like. But maybe that is obvious after playing around with debugify for some hours.
> 
> I'm.. growing increasingly concerned about this as well. I'd like our tests to be reasonably simple to understand. My view is that these tests already require understanding of at least one pass (in this case -instcombine), so it's reasonable to depend on a stable testing pass on top of that. That could well be too optimistic. How much extra effort is involved in parsing this test? Would more documentation help? (Anyone else have thoughts about this?)
> 
> > > Concretely, the advantage of using -debugify in regression tests is that it lets us test an optimization in the exact same place as the optimization's ability to preserve debug info.
> > 
> > I guess there is a test case for the optimization somewhere, is the idea that this test shouuld be added as an extra RUN line in that test case instead of adding a new file?
> 
> Ideally yes. If you grep in tests/Transforms you'll see this pattern used to test gvn, *dce, sccp, licm etc. I would like this patch to adopt the same approach, similar to https://reviews.llvm.org/D48640.
Ok, I think I got it. Thanks to @vsk for the explanations! And sorry @gramanas for having this discussion as part of reviewing your patch.


Repository:
  rL LLVM

https://reviews.llvm.org/D48331