<html><head><style type='text/css'>p { margin: 0; }</style></head><body><div style='font-family: arial,helvetica,sans-serif; font-size: 10pt; color: #000000'><br><hr id="zwchr"><blockquote id="DWT11561" style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; padding-left: 5px; color: rgb(0, 0, 0); font-weight: normal; font-style: normal; text-decoration: none; font-family: Helvetica,Arial,sans-serif; font-size: 12pt;"><b>From: </b>"Geoff Berry" <gberry@codeaurora.org><br><b>To: </b>"Hal Finkel" <hfinkel@anl.gov>, "Daniel Berlin" <dberlin@dberlin.org><br><b>Cc: </b>"Chandler Carruth" <chandlerc@gmail.com>, "Philip Reames" <listmail@philipreames.com>, "Sanjoy Das" <sanjoy@playingwithpointers.com>, mcrosier@codeaurora.org, "llvm-commits" <llvm-commits@lists.llvm.org>, "Junbum Lim" <junbuml@codeaurora.org>, reviews+D19730+public+66d50dddcf8372b8@reviews.llvm.org<br><b>Sent: </b>Friday, May 13, 2016 2:30:11 PM<br><b>Subject: </b>Re: [PATCH] D19730: [BasicAA] Treat llvm.assume as not accessing memory in getModRefBehavior(Function)<br><br>
<p><br>
</p>
<br>
<div class="moz-cite-prefix">On 4/30/2016 12:10 PM, Hal Finkel
wrote:<br>
</div>
<blockquote cite="mid:11633872.232.1462032659314.JavaMail.hfinkel@sapling5.localdomain">
<style>p { margin: 0; }</style>
<div style="font-family: arial,helvetica,sans-serif; font-size: 10pt; color: rgb(0, 0, 0);"><br>
<hr id="zwchr">
<blockquote style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; padding-left: 5px; color: rgb(0, 0, 0); font-weight: normal; font-style: normal; text-decoration: none; font-family: Helvetica,Arial,sans-serif; font-size: 12pt;"><b>From: </b>"Daniel Berlin"
<a class="moz-txt-link-rfc2396E" href="mailto:dberlin@dberlin.org" target="_blank"><dberlin@dberlin.org></a><br>
<b>To: </b>"Hal Finkel" <a class="moz-txt-link-rfc2396E" href="mailto:hfinkel@anl.gov" target="_blank"><hfinkel@anl.gov></a><br>
<b>Cc: </b>"Geoff Berry" <a class="moz-txt-link-rfc2396E" href="mailto:gberry@codeaurora.org" target="_blank"><gberry@codeaurora.org></a>,
"Chandler Carruth" <a class="moz-txt-link-rfc2396E" href="mailto:chandlerc@gmail.com" target="_blank"><chandlerc@gmail.com></a>, "Philip
Reames" <a class="moz-txt-link-rfc2396E" href="mailto:listmail@philipreames.com" target="_blank"><listmail@philipreames.com></a>, "Sanjoy Das"
<a class="moz-txt-link-rfc2396E" href="mailto:sanjoy@playingwithpointers.com" target="_blank"><sanjoy@playingwithpointers.com></a>,
<a class="moz-txt-link-abbreviated" href="mailto:mcrosier@codeaurora.org" target="_blank">mcrosier@codeaurora.org</a>, "llvm-commits"
<a class="moz-txt-link-rfc2396E" href="mailto:llvm-commits@lists.llvm.org" target="_blank"><llvm-commits@lists.llvm.org></a>, "Junbum Lim"
<a class="moz-txt-link-rfc2396E" href="mailto:junbuml@codeaurora.org" target="_blank"><junbuml@codeaurora.org></a>,
<a class="moz-txt-link-abbreviated" href="mailto:reviews+D19730+public+66d50dddcf8372b8@reviews.llvm.org" target="_blank">reviews+D19730+public+66d50dddcf8372b8@reviews.llvm.org</a><br>
<b>Sent: </b>Friday, April 29, 2016 1:49:34 PM<br>
<b>Subject: </b>Re: [PATCH] D19730: [BasicAA] Treat
llvm.assume as not accessing memory in
getModRefBehavior(Function)<br>
<br>
<div dir="ltr"><br>
<div class="gmail_extra"><br>
<div class="gmail_quote">On Fri, Apr 29, 2016 at 10:57 AM,
Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank"></a><a class="moz-txt-link-abbreviated" href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div>
<div style="font-family: arial,helvetica,sans-serif; font-size: 10pt; color: rgb(0, 0, 0);"><br>
<hr>
<blockquote style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; padding-left: 5px; color: rgb(0, 0, 0); font-weight: normal; font-style: normal; text-decoration: none; font-family: Helvetica,Arial,sans-serif; font-size: 12pt;"><b>From: </b>"Daniel Berlin"
<<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>><br>
<b>To: </b>"Hal Finkel" <<a href="mailto:hfinkel@anl.gov" target="_blank"></a><a class="moz-txt-link-abbreviated" href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>><br>
<b>Cc: </b>"Geoff Berry" <<a href="mailto:gberry@codeaurora.org" target="_blank"></a><a class="moz-txt-link-abbreviated" href="mailto:gberry@codeaurora.org" target="_blank">gberry@codeaurora.org</a>>,
"Chandler Carruth" <<a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@gmail.com</a>>,
"Philip Reames" <<a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>>,
"Sanjoy Das" <<a href="mailto:sanjoy@playingwithpointers.com" target="_blank">sanjoy@playingwithpointers.com</a>>,
<a href="mailto:mcrosier@codeaurora.org" target="_blank">mcrosier@codeaurora.org</a>,
"llvm-commits" <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>>,
"Junbum Lim" <<a href="mailto:junbuml@codeaurora.org" target="_blank">junbuml@codeaurora.org</a>>,
<a href="mailto:reviews%2BD19730%2Bpublic%2B66d50dddcf8372b8@reviews.llvm.org" target="_blank">reviews+D19730+public+66d50dddcf8372b8@reviews.llvm.org</a><br>
<b>Sent: </b>Friday, April 29, 2016 12:31:28 PM<br>
<b>Subject: </b>Re: [PATCH] D19730: [BasicAA]
Treat llvm.assume as not accessing memory in
getModRefBehavior(Function)<span class=""><br>
<br>
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><br>
<br>
I'm not sure this is a good idea. As
the comment says, they "never aliases
any particular memory location". This,
however, is not comparing against any
particular location.</blockquote>
<div><br>
</div>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"> It is
asking for general behavior. </blockquote>
<div>Yes, and it neither mod nor refs
anything :)</div>
<div> </div>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">AA should
probably be constant with the function
attributes here. <br>
</blockquote>
<div><br>
</div>
<div>BasicAA already was working around
this. </div>
</div>
</div>
</div>
</span></blockquote>
I know, I'm responsible for the workarounds.</div>
</div>
</blockquote>
<div>;) </div>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div>
<div style="font-family: arial,helvetica,sans-serif; font-size: 10pt; color: rgb(0, 0, 0);"><span class=""><br>
<blockquote style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; padding-left: 5px; color: rgb(0, 0, 0); font-weight: normal; font-style: normal; text-decoration: none; font-family: Helvetica,Arial,sans-serif; font-size: 12pt;">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div> This just puts this knowledge in
one place instead of two (basicaa and
memoryssa).</div>
<div><br>
</div>
<div>In any case, if we are going to do
that, we'd have to actually fix things
properly. Saying assume has
side-effects and writes things was
wrong in the first place compared to
actually modeling control dependencies
for things like assume properly.</div>
<div><br>
</div>
<div>It simply does not have
side-effects or write anything.
Period. Claiming otherwise is a lie
to try to avoid fixing other
infrastructure to handle this kind of
intrinsic properly.</div>
<div><br>
</div>
<div>Until we go and have the
wherewithal to fix that
infrastructure, we are going to have
to accept that we are going to have
things like this, where we are making
up for it.</div>
</div>
</div>
</div>
</blockquote>
</span>I'm well aware of the state of things, and
I agree that we should have a better way of
modeling control dependencies than via memory.<span class=""><br>
<blockquote style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; padding-left: 5px; color: rgb(0, 0, 0); font-weight: normal; font-style: normal; text-decoration: none; font-family: Helvetica,Arial,sans-serif; font-size: 12pt;">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div><br>
</div>
<div>The alternative, which was
currently done, where basicaa and
memoryssa and whoever else has to know
that assume is magic and special,
seems much worse.</div>
<div><br>
</div>
</div>
</div>
</div>
</blockquote>
</span>I understand. However, without our current
system, this still obviously needs to be correct.
What is to prevent a pass from (correctly) seeing
the call to assume, using AA to conclude it does
not actually alter memory, seeing it has no return
values, intrinsics don't throw, and so removing it
as dead?</div>
</div>
</blockquote>
<div><br>
</div>
<div id="DWT2749">Well, it will still return true to
mayHaveSideEffects :)</div>
</div>
</div>
</div>
</blockquote>
True, but this does not mean arbitrary side effects. At the IR
level, our model contains only a fixed set of possible classes
of side effects. We model memory access, unwinding (or other
unknown control-flow effects), and
(possibly-control-flow-dependent) UB (including, but not limited
to, trapping). Unless I'm forgetting something ;), our model
simply does not contain anything else. In fact,
mayHaveSideEffects is not even an independent property, it is
defined like this:<br>
<br>
bool mayHaveSideEffects() const {<br>
return mayWriteToMemory() || mayThrow() || !mayReturn();<br>
}<br>
<br>
To do what we want, we need something else...<br>
<blockquote style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; padding-left: 5px; color: rgb(0, 0, 0); font-weight: normal; font-style: normal; text-decoration: none; font-family: Helvetica,Arial,sans-serif; font-size: 12pt;">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div><br>
</div>
<div>But your question is exactly one of "how do we fix
the infrastructure".</div>
<div>I don't think changing AA, which is an optional
part and supposed to give the best answers it can
(regardless of function attributes or anything else),
is going to affect that much.</div>
<div><br>
</div>
<div>As an aside, note that this is already affecting
things like GlobalsModRef (which also mixes AA and
function attributes), which will now assume that every
function in a callchain with an assume in it modref
things (which is making much worse information :( ). </div>
<div><br>
</div>
<div>It too would have to special case assume to tell it
otherwise.</div>
<div><br>
</div>
<div>This patch should also fix part of this without
breaking assume (the other part it will not due to the
mixing of AA and function attribute calls).</div>
<div><br>
</div>
<div id="DWT2750">However, I'm also happy to put this
knowledge in the 4 places for now if you really
believe that is a better solution.</div>
</div>
</div>
</div>
</blockquote>
It is not really a question of best right now, more like least
bad. I think that we can leave the change as is -- there's no
good reason to move farther away from the solution we want
instead of closer, but the comment needs to be updated to better
reflect the situation. And we need to, sooner rather than later,
introduce some new property or properties that do what we need
here. I think it is fairly dangerous to have, essentially, "AA
says readnone" have different semantics than "the call has the
readnone attribute" in subtle ways.<br>
</div>
</blockquote>
Sorry to have let this slide for a while, but Hal, can you be more
specific about additional information you think needs to be conveyed
in the comment? Something like: the result of this query may not
match similar results derived from function attributes?<br></blockquote>Yes, that would be helpful. Also, the comment you added:<br><br>+ // While the assume intrinsic is marked as arbitrarily writing so that<br>+ // proper control dependencies will be maintained, it never aliases any<br>+ // particular memory location.<br><br>We word 'particular' does not belong here because the query in question does not ask about any particular memory location. Maybe we should just say, ", ... it never access any actual memory locations."?<br><br> -Hal<br><blockquote style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; padding-left: 5px; color: rgb(0, 0, 0); font-weight: normal; font-style: normal; text-decoration: none; font-family: Helvetica,Arial,sans-serif; font-size: 12pt;">
<blockquote cite="mid:11633872.232.1462032659314.JavaMail.hfinkel@sapling5.localdomain">
<div style="font-family: arial,helvetica,sans-serif; font-size: 10pt; color: rgb(0, 0, 0);">The good news is that adding such a thing
might not be that bad. Most code checks mayHaveSideEffects() in
relevant places, and adding another thing that causes that to be
true is not hard. Also, we do now have control-flow-dependent
callsite properties (e.g. isConvergent), and between the two, we
can hopefully find without too much difficultly most of the
places that need to be updated.<br>
<br>
-Hal<br>
<blockquote style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; padding-left: 5px; color: rgb(0, 0, 0); font-weight: normal; font-style: normal; text-decoration: none; font-family: Helvetica,Arial,sans-serif; font-size: 12pt;">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div><br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
<br>
<br>
-- <br>
<div><span></span>Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<span></span><br>
</div>
</div>
</blockquote>
<br>
<pre class="moz-signature">-- <br>Geoff Berry<br>Employee of Qualcomm Innovation Center, Inc.<br> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project<br></pre>
</blockquote><br><br><br>-- <br><div><span name="x"></span>Hal Finkel<br>Assistant Computational Scientist<br>Leadership Computing Facility<br>Argonne National Laboratory<span name="x"></span><br></div></div></body></html>