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

[Hal Finkel via llvm-commits]

On 09/20/2017 11:30 AM, Xinliang David Li wrote:
>
>
> On Wed, Sep 20, 2017 at 3:56 AM, Nuno Lopes via Phabricator 
> <reviews at reviews.llvm.org <mailto:reviews at reviews.llvm.org>> wrote:
>
>     nlopes added a comment.
>
>     In https://reviews.llvm.org/D37832#875262
>     <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.

I don't think this is clear. We may need to pull in a wider audience on 
llvm-dev in order to figure out suitable, consistent semantics.

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

I disagree. In some cases, this is a reasonable approach. In some cases, 
it's not. Whether risking exacerbating an existing problem is a cause 
for blocking progress on other improvements is going to be a matter of 
our professional judgment.

In this case, given that I'm not aware of the bug causing a problem in 
practice, and don't obviously foresee situations in which this 
transformation changes that, if the transformation is useful regardless 
of the foreseeable resolutions, I'm not inclined to hold it up. If, 
however, this transformation is not useful, or not as useful, under some 
resolutions, we might want to figure out the resolution first.

> Besides, what ever exposed problem needs to be fixed in the right place.

Yes.

Thanks again,
Hal

>
> David
>
>
>     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/20170920/fa39ae07/attachment.html>