[PATCH] D19730: [BasicAA] Treat llvm.assume as not accessing memory in getModRefBehavior(Function)

Hal Finkel via llvm-commits llvm-commits at lists.llvm.org
Fri May 13 12:40:41 PDT 2016


----- Original Message -----

> From: "Geoff Berry" <gberry at codeaurora.org>
> To: "Hal Finkel" <hfinkel at anl.gov>, "Daniel Berlin"
> <dberlin at dberlin.org>
> Cc: "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, May 13, 2016 2:30:11 PM
> Subject: Re: [PATCH] D19730: [BasicAA] Treat llvm.assume as not
> accessing memory in getModRefBehavior(Function)

> On 4/30/2016 12:10 PM, Hal Finkel wrote:

> > ----- Original Message -----
> 

> > > 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 >
> > > 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 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?

Yes, that would be helpful. Also, the comment you added: 

+ // While the assume intrinsic is marked as arbitrarily writing so that 
+ // proper control dependencies will be maintained, it never aliases any 
+ // particular memory location. 

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."? 

-Hal 

> > 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

-- 

Hal Finkel 
Assistant Computational Scientist 
Leadership Computing Facility 
Argonne National Laboratory 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160513/36de3dd7/attachment.html>


More information about the llvm-commits mailing list