<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 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" <dberlin@dberlin.org><br><b>To: </b>"Hal Finkel" <hfinkel@anl.gov><br><b>Cc: </b>"Geoff Berry" <gberry@codeaurora.org>, "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, 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">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">hfinkel@anl.gov</a>><br><b>Cc: </b>"Geoff Berry" <<a 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></div><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></div><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><br>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></div><div><br></div></div></div></div>
</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>