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

[David Blaikie]

On Fri, Apr 10, 2015 at 8:22 AM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
>
>
> > On 2015 Apr 10, at 06:50, Diego Novillo <dnovillo at google.com> wrote:
> >
> >
> >
> > On 04/09/15 20:26, Duncan P. N. Exon Smith wrote:
> >
> >> diff --git a/lib/Transforms/Utils/AddDiscriminators.cpp b/lib/Transforms/Utils/AddDiscriminators.cpp
> >> index 0379736..661dc5e 100644
> >> --- a/lib/Transforms/Utils/AddDiscriminators.cpp
> >> +++ b/lib/Transforms/Utils/AddDiscriminators.cpp
> >> @@ -189,7 +189,7 @@ bool AddDiscriminators::runOnFunction(Function &F) {
> >>       // location as B's last instruction (Last), add a new
> >>       // discriminator for First's location and all the instructions
> >>       // in Succ that share the same location with First.
> >> -      if (FirstDIL.atSameLineAs(LastDIL)) {
> >> +      if (FirstDIL == LastDIL) {
> >
> > This will not work as expected. The comparison between the two LOCs need to be necessarily loose. If both LOCs are at the same line number, then we need discriminators to distinguish them.
> > E.g.,
> >
> > 9 ...
> > 10  if (x) y++; else z--;
> > 11 ...
> >
> > We need discriminators for the expressions 'if (x)', 'y++' and 'z--'. They all have different LOCs because they have different column numbers. We need to do the loose comparison 'are they on the same line?'. If so, then each of them gets a new discriminator.
> >
>
>
> Why?  They'll have different line table entries, even in -gmlt.  Here's
> what `llvm-dwarfdump -debug-dump=line` gives with my patch:
>
> 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?
>
> If column info is turned off in -gmlt, I understand why you need a
> discriminator.  Here is what you get with (and without) 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
> 0x0000000000000013      4      0      1   0             1
> 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
>
> But I'd like to understand the following situation, too.  Why do you need
> a discriminator if the scope/inlined-at is different, assuming the
> file/line/column is the same?
>
> 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

Side note: that's weird, why do we have separate line table entries
here? That looks like inefficiency in the line table to me...

>
> Do you need a discriminator here, even though there are separate
> entries?  Why?
>
> > What are you trying to fix? Not sure I understand what's failing here.
>
> I'm in the middle of (well, I hope I'm further than that) reimplementing
> the entire DIDescriptor API, and I came across `atSameLineAs()` and its
> sole consumer.  Nothing's failing, but I want to understand it.