[PATCH] D25872: [IndVarSimplify][DebugLoc] When widening the IV increment, correctly set the debug loc.

[David Blaikie via llvm-commits]

Thanks for the explanation! Sounds OK from my side - though wouldn't mind
an extra set of eyes from someone more familiar with IndVarSimplify, to
check that this is the a good approach for the context.

On Fri, Oct 21, 2016 at 9:24 AM Andrea Di Biagio <andrea.dibiagio at gmail.com>
wrote:

> andreadb added a comment.
>
> Hi David,
>
> > On Fri, Oct 21, 2016 at 3:54 PM, David Blaikie <dblaikie at gmail.com>
> wrote:
> >
> >   I assume we would see this is a pure debug quality bug if you split
> the for loop header over multiple lines? (so it wasn't just a discriminator
> bug)?
>
> You are correct. It is a debug quality issue, and the problem would be
> more evident if the loop increment is on a different line.
>
> >   Which instruction/part of the loop header is it taking the line 4/disc
> 1 from?
>
> It is the SCEVExpander. In particular, it is method
> SCEVExpander::getAddRecExprPHILiterally().
>
> (sorry for the long explanation. I don't know how to explain it briefly
> since the logic is quite convoluted...)
>
> What happens is that we delegate to the SCEVExpander the expansion of a IV
> phi node. We do this by by calling method 'expandCodeFor()' on the
> SCEVExpander object. When we call it, the normalized SCEV for the
> reproducible is: {0,+,1}<nuw><nsw><%for.body>.
>
> The SCEVExpander firstly generates code for the expanded 'start' value. In
> our example, the start value is constant 0, so the expander doesn't have to
> insert new computation in the loop preheader.
> Then SCEVExpander expands the code for the 'step' value. The step value is
> also a constant, so no extra code is inserted in the loop body. At this
> point, the algorithm creates a new phi node with no incoming values; the
> idea is that we populate the new phi node with the "expanded" 'start' value
> and the widened step instruction.
>
> The logic that populates the phi node is in method
> 'SCEVExpander::getAddRecExprPHILiterally()' at around lines 1221:1245.
> For each loop header predecessor, the algorithm adds a widened incoming
> value. In the case of the loop latch basic block, the insertion point for
> the IRBUilder points to the end of the loop latch block
> (Pred->getTerminator()). That means, the new IV increment will
> automatically obtain the debug location from Pred->getTerminator().
>
> In our example, Pred->getTerminator() is:
>
>   br i1 %cmp, label %for.body, label %for.cond.for.end_crit_edge, !dbg !9
>
> Where !dbg !9 is DILocation(line 4, column 3, scope !8).
> !8 is a !DILexicalBlockFile with discriminator 1.
>
> That explains why the widen IV increment is associated to line 4,
> discriminator 1.
>
> I couldn't find a reasonable way to get the debug loc associated to the
> original loop increment from inside method getAddRecExprPHILiterally(). So,
> I eventually opted for this simpler patch.
>
> Again, sorry for the long explanation.
> I hope this makes sense.
>
> -Andrea
>
>
> https://reviews.llvm.org/D25872
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161025/fe2b2507/attachment.html>