[PATCH] D25742: Remove debug location from common tail when tail-merging

[Robert Lougher via llvm-commits]

That's right.  As Paul says, the fundamental issue is that the common-tail
is now ambiguous.  Erasing the source attribution isn't a problem for PGO
as the sample loader uses the maximum instruction weight as the weight of
the basic-block.

Thanks,
Rob.

On 19 October 2016 at 01:04, Robinson, Paul <paul.robinson at sony.com> wrote:

> Discriminators can't help you when the same instruction belongs to two
> different blocks.
>
>
>
> The point (which we should not lose track of) is that when you merge the
> tails of two blocks, the source attribution becomes inherently ambiguous.
> This is not great for debugging and is bad for profiling.  Erasing the
> source attribution means at least we aren't actively lying to either one;
> again it is not great for debugging, although IIUC profiling can cope.  In
> my opinion that makes erasing the source location "less bad" than what we
> have now.
>
>
>
> But, the alternatives appear to be either killing off the optimization
> (removing the ambiguity at a code-size cost) or doing a LOT of work so we
> can attribute multiple source locations to the same instructions (which
> only pushes the problem downstream to the DWARF consumer, who probably has
> no idea how to cope).  (It is not, I have discovered, impossible to
> construct a line table that attributes multiple source locations to the
> same instruction.  But nobody expects to see that.)
>
> --paulr
>
>
>
>
>
> *From:* llvm-commits [mailto:llvm-commits-bounces at lists.llvm.org] *On
> Behalf Of *David Blaikie via llvm-commits
> *Sent:* Tuesday, October 18, 2016 4:34 PM
> *To:* reviews+D25742+public+84a9fdd1c5228b3b at reviews.llvm.org;
> rob.lougher at gmail.com; danielcdh at gmail.com; Pieb, Wolfgang;
> aprantl at apple.com
> *Cc:* llvm-commits at lists.llvm.org
> *Subject:* Re: [PATCH] D25742: Remove debug location from common tail
> when tail-merging
>
>
>
> That's what discriminators are for - but I've no idea where that fits in
> the workflow etc.
>
> In any case I'm not sure it's worth worrying about reusing the same line
> when it happens to be all on the same line (& removing the column - what a
> debugger would do seeing some things with column info and some without etc)
>
>
>
> On Tue., 18 Oct. 2016, 4:27 pm Paul Robinson, <paul.robinson at sony.com>
> wrote:
>
> probinson added a comment.
>
> In https://reviews.llvm.org/D25742#573687, @aprantl wrote:
>
> > In https://reviews.llvm.org/D25742#573401, @probinson wrote:
> >
> > > In https://reviews.llvm.org/D25742#573352, @aprantl wrote:
> > >
> > > > This approach seems generally fine, but I have one question:
> > > >
> > > > If the code were on a single line, and both locations share a common
> ancestor scope, it seems make sense to create a new location using the
> common ancestor scope and line and only remove the column information.
> > >
> > >
> > > That would collapse the if-then-else into (effectively) a single
> statement.  That probably works okay for a debugger but not profiling,
> which still wants to treat the then/else as distinct.  And, after the tail
> merging, the tails are no longer distinct.
> >
> >
> > I'm not sure I understand your point. How is having an orphaned add
> instruction preferable over having it associated with the collapsed
> if-then-else statement? Wouldn't I want that instruction to be counted
> towards the line?
>
>
> No, the profiler wants to assign sample counts to each block
> individually.  By giving each block the same source attribution, you assert
> that they have the same profiles.  That's unlikely to be true in practice.
> Really what happens is that the sample counts would be assigned to the
> parent block, which doesn't help the profiler sort out what to do with the
> nested blocks.
>
> Admittedly, zapping the source attribution on the merged (parts of the)
> blocks doesn't let you attribute those counts to *any* block, but at least
> you aren't attributing counts incorrectly.
>
> Robert may want to correct some of my assumptions here, but this is my
> understanding.
>
>
> https://reviews.llvm.org/D25742
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161019/8923219d/attachment.html>