[PATCH] D19730: [BasicAA] Treat llvm.assume as not accessing memory in getModRefBehavior(Function)
Geoff Berry via llvm-commits
llvm-commits at lists.llvm.org
Fri May 13 12:30:11 PDT 2016
On 4/30/2016 12:10 PM, Hal Finkel wrote:
>
> ------------------------------------------------------------------------
>
> *From: *"Daniel Berlin" <dberlin at dberlin.org>
> *To: *"Hal Finkel" <hfinkel at anl.gov>
> *Cc: *"Geoff Berry" <gberry at codeaurora.org>, "Chandler Carruth"
> <chandlerc at gmail.com>, "Philip Reames"
> <listmail at philipreames.com>, "Sanjoy Das"
> <sanjoy at playingwithpointers.com>, mcrosier at codeaurora.org,
> "llvm-commits" <llvm-commits at lists.llvm.org>, "Junbum Lim"
> <junbuml at codeaurora.org>,
> reviews+D19730+public+66d50dddcf8372b8 at reviews.llvm.org
> *Sent: *Friday, April 29, 2016 1:49:34 PM
> *Subject: *Re: [PATCH] D19730: [BasicAA] Treat llvm.assume as not
> accessing memory in getModRefBehavior(Function)
>
>
>
> On Fri, Apr 29, 2016 at 10:57 AM, Hal Finkel <hfinkel at anl.gov
> <mailto:hfinkel at anl.gov>> wrote:
>
>
> ------------------------------------------------------------------------
>
> *From: *"Daniel Berlin" <dberlin at dberlin.org
> <mailto:dberlin at dberlin.org>>
> *To: *"Hal Finkel" <hfinkel at anl.gov <mailto:hfinkel at anl.gov>>
> *Cc: *"Geoff Berry" <gberry at codeaurora.org
> <mailto:gberry at codeaurora.org>>, "Chandler Carruth"
> <chandlerc at gmail.com <mailto:chandlerc at gmail.com>>,
> "Philip Reames" <listmail at philipreames.com
> <mailto:listmail at philipreames.com>>, "Sanjoy Das"
> <sanjoy at playingwithpointers.com
> <mailto:sanjoy at playingwithpointers.com>>,
> mcrosier at codeaurora.org <mailto:mcrosier at codeaurora.org>,
> "llvm-commits" <llvm-commits at lists.llvm.org
> <mailto:llvm-commits at lists.llvm.org>>, "Junbum Lim"
> <junbuml at codeaurora.org <mailto:junbuml at codeaurora.org>>,
> reviews+D19730+public+66d50dddcf8372b8 at reviews.llvm.org
> <mailto:reviews%2BD19730%2Bpublic%2B66d50dddcf8372b8 at reviews.llvm.org>
> *Sent: *Friday, April 29, 2016 12:31:28 PM
> *Subject: *Re: [PATCH] D19730: [BasicAA] Treat llvm.assume
> as not accessing memory in getModRefBehavior(Function)
>
>
>
> 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.
>
>
> It is asking for general behavior.
>
> Yes, and it neither mod nor refs anything :)
>
> AA should probably be constant with the function
> attributes here.
>
>
> BasicAA already was working around this.
>
> I know, I'm responsible for the workarounds.
>
> ;)
>
>
> This just puts this knowledge in one place instead of two
> (basicaa and memoryssa).
>
> 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.
>
> 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.
>
> 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.
>
> 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.
>
>
> 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.
>
> 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?
>
>
> Well, it will still return true to mayHaveSideEffects :)
>
> 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:
>
> bool mayHaveSideEffects() const {
> return mayWriteToMemory() || mayThrow() || !mayReturn();
> }
>
> To do what we want, we need something else...
>
>
> But your question is exactly one of "how do we fix the
> infrastructure".
> 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.
>
> 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 :( ).
>
> It too would have to special case assume to tell it otherwise.
>
> 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).
>
> However, I'm also happy to put this knowledge in the 4 places for
> now if you really believe that is a better solution.
>
> 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.
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?
> 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.
>
> -Hal
>
>
>
>
>
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
--
Geoff Berry
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160513/e6140216/attachment.html>
More information about the llvm-commits
mailing list