[PATCH] D37832: Eliminate PHI (int typed) which is only used by inttoptr

[Xinliang David Li via llvm-commits]

On Fri, Sep 15, 2017 at 8:11 PM, Hal Finkel <hfinkel at anl.gov> wrote:

>
> On 09/15/2017 09:59 PM, Xinliang David Li wrote:
>
>
>
> On Fri, Sep 15, 2017 at 7:47 PM, Hal Finkel via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
>> hfinkel added inline comments.
>>
>>
>> ================
>> Comment at: lib/Transforms/InstCombine/InstCombinePHI.cpp:72
>> +//    int_ptr = BitCast(ptr_ptr)
>> +//    int_init = Load(int_ptr)
>> +//    br label %bb2
>> ----------------
>> davidxl wrote:
>> > davidxl wrote:
>> > > hfinkel wrote:
>> > > > I don't understand why you're looking for a load here specifically.
>> It's a good example, because it shows that you might need to look for
>> something here other than a ptrtoint, but that seems to motivate taking a
>> general input (not specifically looking for this load as you do below).
>> > > Checking for loads because instcombine can fold the inserted inttoptr
>> away. Same for PHI defs. For other instructions, we are basically just
>> hoisting inttoptr above to the PHI operand, which is unclear it is a win.
>> > Also general input is already handled first -- by looking in forward
>> direction if there is an existing inttoptr that can be reused.
>> >
>> > bb1:
>> > %arg_int64 = <general operation>
>> > ...
>> > %ptr_val = inttoptr i64 %arg_int64 to float*
>> >
>> > ...
>> > bbm:
>> >   %int64_phi = PHI ([%arg_int64, %bb1], [...])
>> >    %ptr_val = inttoptr i64 %int64_phi to float*
>> >
>> > ==>
>> >
>> >    %ptr_val = PHI([%ptr_val, %bb1], [...])
>> >
>> >
>> >
>> > Checking for loads because instcombine can fold the inserted inttoptr
>> away. Same for PHI defs. For other instructions, we are basically just
>> hoisting inttoptr above to the PHI operand, which is unclear it is a win.
>>
>> Seems like a win to me. It will make AA more powerful in the loop.
>
>
>
> In fact, my previous version of the patch does that -- it actually checks
> if we can hoist intptr out of the loop. Unfortunately it does not work well
> --> for some reason, the LoopInfo is not properly updated, so I could not
> eliminate the intptr properly which puzzled me for a while.  It could be a
> bug with the new PM, but I did not have time to track it down.
>
> For now I prefer handling the limited case first and follow up with more
> general cases when the need arises. Doing this incrementally is also less
> risky. Does it sound fine to you?
>
>
> I don't understand why any of these things would need to update LoopInfo:
> The only state that LoopInfo keeps is the list of blocks in the loop and
> the list of subloops. Neither of which this changes. If your more-general
> patch triggers a bug with LoopInfo, wouldn't this one too (just less often
> because the transformation triggers less often)?
>


I wasn't clear. What I meant that the LoopInfo passed to the InstCombine is
already incorrect. The issue does not exist when running instcombine in
isolation with opt, but shown with more complicated pipelines.  That is
certainly an orthogonal issue.

David



>
> Thanks again,
> Hal
>
>
>
> thanks,
>
> David
>
>
>
>
>
>> Besides, we can have a separate combine for
>>   inttoptr(load(bitcast(pp))) => load(pp)
>>
>>
>
>
>
>> (assuming we can prove what's necessary to make AA on these pointers safe)
>>
>>
>> https://reviews.llvm.org/D37832
>>
>>
>>
>>
>
> --
> Hal Finkel
> Lead, Compiler Technology and Programming Languages
> Leadership Computing Facility
> Argonne National Laboratory
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170915/72bdf35b/attachment.html>