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

[Xinliang David Li via llvm-commits]

On Wed, Sep 20, 2017 at 3:56 AM, Nuno Lopes via Phabricator <
reviews at reviews.llvm.org> wrote:

> nlopes added a comment.
>
> In https://reviews.llvm.org/D37832#875262, @davidxl wrote:
>
> > As I have mentioned, this patch itself does *not* fold any
> ptrtoint/inttoptr. It simply moves the intptr across the phi node. The
> folding you see with the test cases  is done by existing optimizations, so
> I am not sure what the objection is about.
>
>
> Ok, to be precise the patch does the following:
> v = phi(ptr2int(p), ptr2int(q))
>  =>
> v = bitcast(phi(p, q) to int)
>
>

Should be :

v = phi(ptr2int(p), ptr2int(q))
 =>
v = ptrtoint(phi(p, q) to int)


Note that CreateBitOrPointerCast will produce either bitcast or inttoptr or
ptrtoint depending on the types.

This transformation by itself is not correct in all cases. Ptr2int is not a
> NOP.
>

Ptr2int is a nop if the size of the input and output type is the same.



> Also, it makes me really uneasy the fact that the test cases provided take
> advantage of a broken optimization.



Are you referring to PR/34548. See the discussions there. There is no
evidence that the inttoptr/ptrtoint folding is to be blamed.  It is the
result of CSE that confuses the AA.


> Since it's not possible to test the new transformation in isolation, all I
> see is an end-to-end incorrect transformation (even if the proposed
> transformation was fully correct, which is not).
>
>
It is fair that you worry that this can expose existing problems -- but
that needs to be discussed case by case when the bug actually get exposed.
Besides, what ever exposed problem needs to be fixed in the right place.

David


>
> https://reviews.llvm.org/D37832
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170920/703193f1/attachment.html>