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

[Adrian Prantl via llvm-commits]

> On Oct 18, 2016, at 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 <https://reviews.llvm.org/D25742>
I still don't quite understand it, but I would really like to :-)

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.

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.

I think that my problem is that I don't yet understand how the profiler maps source locations to basic blocks. 

How does the profiler that you are using map from source locations to basic blocks? Why would it not be able to attribute the common tail to the successor block?

thanks for you patience,
adrian
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161019/4ac82b8f/attachment.html>