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

[Duncan P. N. Exon Smith]

> 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?

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.

> 
>> E.g., your example in -g but without column info would give this with my
>> patch:
>> 
>> Address            Line   Column File   ISA Discriminator Flags
>> ------------------ ------ ------ ------ --- ------------- -------------
>> 0x0000000000000000      2      0      1   0             0  is_stmt
>> 0x0000000000000007      4      0      1   0             0  is_stmt prologue_end
>> 0x000000000000000b      4      0      1   0             0
>> 0x0000000000000013      4      0      1   0             0
>> 0x0000000000000019      5      0      1   0             0  is_stmt
>> 0x000000000000001f      7      0      1   0             0  is_stmt
>> 0x0000000000000021      7      0      1   0             0  is_stmt end_sequence
>> 
>> Do you need a discriminator here, even though there are separate
>> entries?  Why?
> 
> Not in this case. The problem is that we cannot always reliably use
> this entry equivalence. In some cases (even with column info on),
> entry equivalence will be the wrong test. There will be two distinct
> basic blocks with the exact same LOC entry.
> 
> Yeah, it's kind of irritating.

Also, David and Adrian pointed out that these *shouldn't* be separate
line table entries.  So I see why you need a discriminator here.