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

Wed Oct 19 11:36:37 PDT 2016

On Wed, Oct 19, 2016 at 11:30 AM Robert Lougher <rob.lougher at gmail.com>
wrote:

> Yes, we could use a new discriminator to differentiate the common-tail
> from the original blocks.  As there will be no source correspondence the
> counts will be ignored by the sample loader (it maps from source).  Talking
> to the debugger people it seems they don't take any notice of the
> discriminator so the fact it doesn't match the source won't be a problem.
> However, I don't know if this is true for all consumers.  The question is,
> is it worth doing for what is a fairly rare special case?
>

For my money I doubt it's worthwhile

>
> Thanks,
> Rob.
>
>
> On 19 October 2016 at 16:51, Robinson, Paul <paul.robinson at sony.com>
> wrote:
>
> Discriminators can't help you when the same instruction belongs to two
> different blocks.
>
>
> What I meant was - you were talking about the ambiguity of keeping it on
> the same line, when sunk into the common tail block (if it happens to be on
> the same line) - that that would make the profile ambiguous/confused
> because now all 3 blocks are the same location. Discriminators disambiguate
> those 3 blocks that are all on the same line (the if, the else, and the
> common successor).
>
> Discriminators let you say that two different instructions belong to two
> different blocks even though they have the same source location.  If we
> ignore columns for a single-line if-then-else then yes we can (do!) use
> discriminators exactly that way.
>
>
>
> If we postulate a transformation from
>
> if (x) stmt1; else stmt2;
>
> into
>
> { if (x) stmt1-prefix; else stmt2-prefix; common-suffix; }
>
> then you could in principle use discriminators to distinguish the original
> two blocks and the artificial third block.
>
> However that artificial third block does not unambiguously map back to any
> source block.  It does not help weight the branch decision that chooses
> between stmt1 and stmt2, and the *relative* weight is really what matters
> here.
>
>
>
> I don't know what the profile analysis does with hits on the common
> suffix; one hopes that they are somehow treated the same as hits on the
> instructions that are evaluating the 'x' condition, i.e. counting toward
> the block containing the original 'if'.  With sufficient bookkeeping I
> suppose we'd be able to tag the common suffix with the source location of
> the parent 'if' but it's doubtful we could really make that work.
>
> --paulr
>
>
>
> *From:* David Blaikie [mailto:dblaikie at gmail.com]
> *Sent:* Tuesday, October 18, 2016 5:51 PM
> *To:* Robinson, Paul;
> 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
>
>
>
>
>
> On Tue, Oct 18, 2016 at 5:04 PM Robinson, Paul <paul.robinson at sony.com>
> wrote:
>
> Discriminators can't help you when the same instruction belongs to two
> different blocks.
>
>
> What I meant was - you were talking about the ambiguity of keeping it on
> the same line, when sunk into the common tail block (if it happens to be on
> the same line) - that that would make the profile ambiguous/confused
> because now all 3 blocks are the same location. Discriminators disambiguate
> those 3 blocks that are all on the same line (the if, the else, and the
> common successor).
>
> But anyway - there's no great answer for any of this, but I'm happy enough
> to use profiling as a guide for what the right location is (even if,
> arguably, the experience for debugger users might be a bit weird - not
> having certain lines of code execute, etc - because the alternative would
> just be differently weird). & I don't think it's really worth optimizing
> for the "these two blocks happen to be on the same line so we can use the
> line and drop the column"
>
> - Dave
>
>
>
>
> 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/d2fe4bd6/attachment.html>