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

[Alexey Samsonov]

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

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

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


>
>
>> > It will be "mostly" right for "depth 1" phi translation, however.
>>
>>
>>
>>
>>
>> http://reviews.llvm.org/D10351
>>
>> EMAIL PREFERENCES
>>   http://reviews.llvm.org/settings/panel/emailpreferences/
>>
>>
>>


-- 
Alexey Samsonov
vonosmas at gmail.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150610/1d19dff8/attachment.html>