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

[Eric Christopher]

Since I'm not sure you're on IRC...

If we have column information that's just as good as file and line for
discriminating. If those are also the same and we have different basic
blocks then we need a discriminator. I don't think that anyone is trying to
remove discriminators, just if we have column information we can use that
too?

-eric

On Fri, Apr 10, 2015 at 10:10 AM Diego Novillo <dnovillo at google.com> wrote:

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