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

[Daniel Berlin via llvm-commits]

I'm with Hal that we've definitely tried to hold the house of cards
together, while acknowledging it is a house of cards.
While unfortunate, it also turns into the way these things often get fixed.
:(


On Wed, Sep 20, 2017 at 12:34 PM, Hal Finkel via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

>
> 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> 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.
>
>
> 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
>>
>>
>>
>>
>
> --
> Hal Finkel
> Lead, Compiler Technology and Programming Languages
> Leadership Computing Facility
> Argonne National Laboratory
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170920/be1aca91/attachment.html>