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

[Diego Novillo]

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.

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


Diego.