<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="gmail-"><br>
In <a href="https://reviews.llvm.org/D37832#875262" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D37832#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><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. Besides, what ever exposed problem needs to be fixed in the right place.</div><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/<wbr>D37832</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>