[PATCH] [GVN] Set proper debug locations for some instructions created by GVN.

[Daniel Berlin]

On Wed, Jun 10, 2015 at 12:22 AM, Alexey Samsonov <vonosmas at gmail.com> wrote:
>
>
> On Tue, Jun 9, 2015 at 6:41 PM, Daniel Berlin <dberlin at dberlin.org> wrote:
>>
>>
>>
>> On Tue, Jun 9, 2015 at 5:53 PM Alexey Samsonov <vonosmas at gmail.com> wrote:
>>>
>>> In http://reviews.llvm.org/D10351#185951, @dberlin wrote:
>>>
>>> > No, they aren't, and it will in fact, often get the wrong location for
>>> > this.
>>> >
>>> > But given how GVN works, and the expectation that it has that
>>> >  phitransaddr will insert instructions for it,
>>> >  this is pretty much the best you can do (unless you want to do
>>> > nothing).
>>> >
>>> > This is because PHITransAddr does not have any clue in the world about
>>> >  what computation it is really going to eventually replace, and GVN
>>> >  can't tell it, because GVN doesn't know either.
>>> >
>>> > (Basically, GVN is asking phi translate "here's a load, translate all
>>> >  of it's parts". PHI Translate doesn't just do phi translation, it will
>>> >  in fact, go off looking to see if something might have the same
>>> >  operands as the result, etc).
>>>
>>>
>>> Right... But why is this wrong?
>>
>>
>> Wrong is a strong term. There are no right answers here unless you do
>> piece-by-piece insertion of the original computation.
>>
>>>
>>> Suppose GVN replaces a Load L in the basic block BB_3,
>>> and the address computation for L involves values V1 from BB_1 (with
>>> location Loc_1) and V2 from BB_2 (with location Loc_2).
>>
>>
>> This is the simple "depth 1" case i mentioned will work.
>>
>>> PHITransAddr basically just re-creates the address computation in some
>>> predecessor of BB_3.
>>
>>
>> basically is the part that is right.
>> But what it *inserts* is not always  1:1 equivalent with the original set
>> of expressions that generate the address computation.
>>
>> If it was just doing "create original address computation piece by piece",
>> it would always be correct to associate them with the equivalent values.
>> But as you can see, it does simplification, rearrangement, and equivalent
>> expression finding.
>>
>> This means the input inst might not always be the best choice of location,
>> as the computation it has inserted might not be doing the part input inst
>> did at all. The only guarantee you get is that the end result produces the
>> same value. Not that it does it the same way in the same pieces in the same
>> order.
>>
>> (It may also cause debug locations that seriously jump around when it
>> reuses existing expressions willy-nilly).
>>>
>>> Shouldn't instructions re-computing V1 and V2 be associated with the same
>>> locations Loc_1 and Loc_2 as the original values?
>>
>>
>> Yes.
>> But the relationship of the input Inst computation  to what it *inserts*
>> is not 1 to 1
>
>
> Agree, thanks for clarifications. Given all the moving parts, it's hard to
> do something simple and smart about choosing
> the correct locations for inserted instructions (and I'd really want *some*
> locations attached to them).
>

100% agreed

> If we attach location of a load instruction being replaced to all the
> instructions created by PHITransAddr, it woudln't
> be nice either ((a) it's easier to attach DebugLoc as soon as we created the
> instruction, and (b) it will also cause
> debug locations jump around - predecessor BB can be hundreds of BBs away).

Agreed

> Do you think it's fine to proceed with the given patch, or we'd better do
> something different?

No, i think you should proceed with this one.
I don't think it's worth trying to do something better here ATM,
because it seems really hard to come up with an answer that is
"strictly better" and not just "different" :)