[PATCH] WIP: AddDiscriminators: Check more carefully for equivalent locations

[Diego Novillo]

On Fri, Apr 10, 2015 at 12:59 PM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
>
>> On 2015 Apr 10, at 09:48, Diego Novillo <dnovillo at google.com> wrote:
>>
>> On Fri, Apr 10, 2015 at 11:22 AM, Duncan P. N. Exon Smith
>> <dexonsmith at apple.com> wrote:
>>
>>> Why?  They'll have different line table entries, even in -gmlt.  Here's
>>> what `llvm-dwarfdump -debug-dump=line` gives with my patch:
>>
>> Because column information cannot be reliably used to distinguish
>> multiple basic blocks on the same line.  In this example we are
>> discussing, it can, but in other circumstances it is not possible.
>> The bigger issues are macro expansions and compiler generated builtin
>> expansions (say, memcpy). There are other examples in the original
>> dwarf proposal (http://wiki.dwarfstd.org/index.php?title=Path_Discriminators).
>>
>> This is why, we don't even try to use columns. The logic in the sample
>> profiler uses line number and discriminator exclusively because of the
>> various limitations that column info has. So, if two distinct basic
>> blocks share the same line number, we make sure that they get
>> different discriminator numbers.
>>
>>> Address            Line   Column File   ISA Discriminator Flags
>>> ------------------ ------ ------ ------ --- ------------- -------------
>>> 0x0000000000000000      2      0      1   0             0  is_stmt
>>> 0x0000000000000007      4      9      1   0             0  is_stmt prologue_end
>>> 0x000000000000000b      4      7      1   0             0
>>> 0x0000000000000013      4     21      1   0             0
>>> 0x0000000000000016      4     19      1   0             0
>>> 0x0000000000000019      5      9      1   0             0  is_stmt
>>> 0x000000000000001c      5      7      1   0             0
>>> 0x000000000000001f      7      1      1   0             0  is_stmt
>>> 0x0000000000000021      7      1      1   0             0  is_stmt end_sequence
>>>
>>> 0xb is the end of the one basic block, and 0x13 is the start of the next.
>>> I can easily tell them apart ;).  Why do you need a discriminator?  Without
>>> my patch, AddDiscriminators would set the discriminator to `1` on the 0x13
>>> entry.  Why?
>>
>> Because of the limitations with column information. The sample profile
>> pass will not even try to use column numbers. If the code came from
>> something like this:
>>
>> #define CMP_INC(x, y) if (x > 0) x++; else y--
>>
>> int foo(int x, int y) {
>>  CMP_INC(x,y);
>>  CMP_INC(y,x);
>> }
>>
>> Even with column information, you get the two expansions of CMP_INC at column 3.
>
> I don't understand.  Why wouldn't you use column information when
> it's there?

Because we can't trust it (like in the macro expansion case).

> IIUC, then by your logic, you shouldn't even check if they're on the
> same line or have the same filename; you should just always add a
> discriminator.

No, that would be unnecessary. You only need a discriminator when the
file and line are the same.


Diego.