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


Repository:
  rL LLVM

https://reviews.llvm.org/D48331