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

[Hal Finkel via llvm-commits]

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 <mailto: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)?

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 <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/09ac39fb/attachment.html>