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

[Robinson, Paul via llvm-commits]

(Re-adding llvm-commits)

My idea was to remove the column and use the *scope* from the "if" iff "then" and "else" block are on the same line.

My misunderstanding, sorry... although the line table doesn't know about scopes so I'd think it would come down to the same thing.

A better example that is more likely to appear on one line is the ternary ?: operator. In C++11 with lambdas you can also end up with a surprising amount of control flow condensed to a single line, though I can't think of a good example where tail-merging would apply.

Oh, something like 'return x ? foo(1) : foo(2);' would do it.  Setting up the parameters is disjoint but the actual call and passing back the return value would be a common tail.

That said I understand this is a micro-optimization and perhaps not worth doing.

Yeah, I think we're all agreed on that point.
--paulr

From: aprantl at apple.com [mailto:aprantl at apple.com]
Sent: Wednesday, October 19, 2016 10:49 AM
To: Robinson, Paul
Cc: reviews+D25742+public+84a9fdd1c5228b3b at reviews.llvm.org; danielcdh at gmail.com; Pieb, Wolfgang
Subject: Re: [PATCH] D25742: Remove debug location from common tail when tail-merging


On Oct 19, 2016, at 10:30 AM, Robinson, Paul <paul.robinson at sony.com<mailto:paul.robinson at sony.com>> wrote:

Let me rephrase what I think you said in my own words and correct me at the point where I drift off.
The point of profiling is to assign weights to each basic block. When an instruction that exists in both a then and an else branch is sunk into their common successor block, and it would somehow keep both locations, then each time the instruction is executed, the counter for the then and the else block would be incremented simultaneously thus incorrectly increasing the weight of one of them each time.

If we could manage to associate both locations with the same instruction, yes each hit would be attributed to both blocks (incorrectly).  Currently of course we associate only one location (arbitrarily picking one), so all hits are attributed to one block even if the execution was actually a result of the other block.

My proposal was to assign a new location to the common tail that keeps the source line, but uses the lexical scope of the parent block of the single-line if-then-else statement.
In a conversation I had with Dehao last week I learned that samplepgo, for example only identifies source locations by file/line/discriminator, so in order to make my proposal work with samplepgo, we would have to assign a new discriminator to the location of the common tail, because it doesn't look at scopes.

This would clearly distinguish hits on the common tail versus hits on the original then/else blocks.  At best you could assign the hit to the 'if' as a whole but not to either of the then/else blocks.  Assuming the 'if' was itself in a nested block of some kind, this would ensure the hit was assigned somewhere useful.  I don't know whether the current sample analysis can assign the hit somewhere useful if we give the tail to line 0, as this patch (ultimately) wants to do.

But I think we really *don't* want to assign the tail to the location of the 'if' unless the entire if/then/else is on a single line,

Sure, and I don't think anyone proposed this. My idea was to remove the column and use the *scope* from the "if" iff "then" and "else" block are on the same line.


because that will mess up debugging.  And as David pointed out, single-line if/then/else is something of a special and unusual case, probably not worth addressing separately.

A better example that is more likely to appear on one line is the ternary ?: operator. In C++11 with lambdas you can also end up with a surprising amount of control flow condensed to a single line, though I can't think of a good example where tail-merging would apply.

That said I understand this is a micro-optimization and perhaps not worth doing.

-- adrian

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161019/5ca04961/attachment.html>