<div dir="ltr">I'm with Hal that we've definitely tried to hold the house of cards together, while acknowledging it is a house of cards.<div>While unfortunate, it also turns into the way these things often get fixed.</div><div>:(</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Sep 20, 2017 at 12:34 PM, Hal Finkel via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"><span class="">
<br>
<div class="m_-5313889579487802035moz-cite-prefix">On 09/20/2017 11:30 AM, Xinliang David
Li wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr"><br>
<div class="gmail_extra"><br>
<div class="gmail_quote">On Wed, Sep 20, 2017 at 3:56 AM, Nuno
Lopes via Phabricator <span dir="ltr"><<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">nlopes added a comment.<br>
<span class="m_-5313889579487802035gmail-"><br>
In <a href="https://reviews.llvm.org/D37832#875262" rel="noreferrer" target="_blank">https://reviews.llvm.org/D3783<wbr>2#875262</a>,
@davidxl wrote:<br>
<br>
> 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.<br>
<br>
<br>
</span>Ok, to be precise the patch does the following:<br>
v = phi(ptr2int(p), ptr2int(q))<br>
=><br>
v = bitcast(phi(p, q) to int)<br>
<br>
</blockquote>
<div><br>
</div>
<div><br>
</div>
<div>Should be :</div>
<div><br>
</div>
<div>
<div>v = phi(ptr2int(p), ptr2int(q))</div>
<div> =></div>
<div>v = ptrtoint(phi(p, q) to int)</div>
</div>
<div> <br>
</div>
<div><br>
</div>
<div>Note that CreateBitOrPointerCast will produce either
bitcast or inttoptr or ptrtoint depending on the types.</div>
<div><br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
This transformation by itself is not correct in all cases.
Ptr2int is not a NOP.<br>
</blockquote>
<div><br>
</div>
<div>Ptr2int is a nop if the size of the input and output
type is the same.</div>
<div><br>
</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Also, it makes me really uneasy the fact that the test
cases provided take advantage of a broken optimization. </blockquote>
<div><br>
</div>
<div><br>
</div>
<div>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.</div>
</div>
</div>
</div>
</blockquote>
<br></span>
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.<span class=""><br>
<br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">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).<br>
<br>
</blockquote>
<div><br>
</div>
<div>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.</div>
</div>
</div>
</div>
</blockquote>
<br></span>
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.<br>
<br>
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.<span class=""><br>
<br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div> Besides, what ever exposed problem needs to be fixed
in the right place.</div>
</div>
</div>
</div>
</blockquote>
<br></span>
Yes.<br>
<br>
Thanks again,<br>
Hal<br>
<br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div><br>
</div>
<div>David</div>
<div> <br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<a href="https://reviews.llvm.org/D37832" rel="noreferrer" target="_blank">https://reviews.llvm.org/D3783<wbr>2</a><br>
<br>
<br>
<br>
</blockquote>
</div>
<br>
</div>
</div>
</blockquote><span class="">
<br>
<pre class="m_-5313889579487802035moz-signature" cols="72">--
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory</pre>
</span></div>
<br>______________________________<wbr>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div>