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

[Duncan P. N. Exon Smith]

> 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

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.