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

[David Blaikie]

On Fri, Apr 10, 2015 at 9:48 AM, 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.


I don't really understand why line information is any more or less reliable
than column information, though?


>   In this example we are
> discussing, it can, but in other circumstances it is not possible.
>

Certainly that's true - that's why we need discriminators, for when things
aren't /otherwise/ disambiguated (eg: if they're on different lines they're
different, that's easy - it's when it's two basic blocks on the same line
that it gets hard - but if, again, they're in different columns that seems
as good as when they're on different lines - and again, falling back to
discriimnators when they happen to be at the same column as well)


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


That description doesn't seem to mention column info or its limitations...
does it? (I just searched for column, the only mention I can find is
describing the line table itself and the discriminator column, not column
info)


> 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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150410/a343a4ca/attachment.html>