[PATCH] D25542: GVN-hoist: avoid calling MemoryLocation::get() on a call (PR30499)

Tue Nov 8 12:33:08 PST 2016

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

> From: "Daniel Berlin" <dberlin at dberlin.org>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: reviews+D25542+public+6ffd7c85a26c1a77 at reviews.llvm.org,
> "llvm-commits" <llvm-commits at lists.llvm.org>, "Sebastian Pop"
> <sebpop at gmail.com>
> Sent: Monday, November 7, 2016 8:08:37 PM
> Subject: Re: [PATCH] D25542: GVN-hoist: avoid calling
> MemoryLocation::get() on a call (PR30499)

> On Mon, Nov 7, 2016 at 5:37 PM, Hal Finkel < hfinkel at anl.gov > wrote:

> > Hi Danny,
> 

> > MemoryLocation does not apply to calls directly because a call can
> > easily affect multiple locations.
> 
> Yes, but people often just want to know "does instruction
> def/ref/nothing the memory of other instruction"

> They don't often want to care about exactly what type of instruction
> they are holding in their hand, and at least right now, it doesn't
> just work all the time, you have to know what you are holding in
> your hands.

> They also don't often care about specific locations.

> If we think we can make that work all the time, awesome, let's do
> that.
This is exactly my point. I see no reason not to make this "just work." Normally, this is exactly what you want: I have Instruction *I1 and Instruction *I2, tell me about the aggregated relative aliasing of any memory they access. The fact that we can't do this I don't think came from any particular design, just from the way the interface has evolved. 

FWIW, We also currently have an issue that our mod/ref interface does not give us MayAlias vs MustAlias differentiation (etc.). So there are some use cases that require using the underlying MemoryLocation-based interface. There are other potential uses for MemoryLocation, such as when you want to take the address and change the size (or the offset, but unfortunately, we can't do this right now). 

> Otherwise, literally everyone i know who has ever written a memory
> optimization has run into this issue :P
Yes, and having written a significant amount of this code myself, I too have found the state of things unfortunate. 

-Hal 

> (The further problem here being the one you expect, we expect people
> to manually phi translate things like pointers, which means if they
> want the best answers, they have to try to use MemoryLocation when
> they can, and phi translate it appropriately)

> Don't get me wrong, there are times when people want *more* detailed
> answers (ie the second getmodrefinfo you have below) , but from what
> i've seen, that's pretty rare.

> > We could add a MemoryLocations (plural), but we'd also want to
> > capture the read/write sense of each location, and then we still
> > run
> > the risk of losing the ability to capture specific call-vs-call
> > aliasing logic.
> 

> > We have:
> 

> > ModRefInfo AAResults::getModRefInfo(ImmutableCallSite CS1,
> 
> > ImmutableCallSite CS2);
> 

> > and
> 

> > ModRefInfo AAResults::getModRefInfo(ImmutableCallSite CS,
> 
> > const MemoryLocation &Loc);
> 

> > and I wonder whether it would be better to encourage developers to
> > always use the mod/ref interfaces when we might be dealing with
> > calls (as single entities). I suspect that we should probably be
> > using mod/ref more than we do now anyway.
> 

> > I think that we should fix the mod/ref functions so that all of the
> > various combinations of underlying instruction types just work.
> 

> > -Hal
> 

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

> > > From: "Daniel Berlin" < dberlin at dberlin.org >
> 
> > > To: "Sebastian Pop" < sebpop at gmail.com >
> 
> > > Cc: reviews+D25542+public+6ffd7c85a26c1a77 at reviews.llvm.org ,
> > > "llvm-commits" < llvm-commits at lists.llvm.org >, "Hal
> 
> > > Finkel" < hfinkel at anl.gov >
> 
> > > Sent: Wednesday, October 12, 2016 10:40:48 PM
> 
> > > Subject: Re: [PATCH] D25542: GVN-hoist: avoid calling
> > > MemoryLocation::get() on a call (PR30499)
> 
> > >
> 
> > >
> 
> > > (as an aside, Hal, i'd love your opinion on whether
> > > MemoryLocation
> 
> > > should really be something that handles calls as well. Or,
> 
> > > alternatively, at least getModRefInfo uses something that does
> 
> > > without people having to know about it.
> 
> > >
> 
> > >
> 
> > > If we used something like MemoryLocOrCall [obviously this is just
> > > a
> 
> > > simple union class]), we could simplify parts of the AA API. I
> > > also
> 
> > > wouldn't need it to hack around things in the AA API's
> 
> > > getModRefInfo.
> 
> > >
> 
> > >
> 
> > >
> 
> > > Right now we have this artificial distinction where we have
> > > things
> 
> > > that use pointers get MemoryLocations, other things get
> 
> > > ImmutableCallSites. This kinda makes sense for alias() (though
> > > more
> 
> > > advanced aa's are just implementing that in terms of mod ref info
> 
> > > they have created).
> 
> > >
> 
> > >
> 
> > > However, the current AA API operates on both *anyway*, and the
> 
> > > overloads you get are confusing.
> 
> > >
> 
> > >
> 
> > > IE we have:
> 
> > > 00333 ModRefInfo getModRefInfo ( ImmutableCallSite CS , const
> 
> > > MemoryLocation &Loc);
> 
> > > and
> 
> > > 00751 ModRefInfo getModRefInfo ( ImmutableCallSite CS1,
> 

> > > ImmutableCallSite CS2);
> 
> > >
> 
> > >
> 
> > > etc
> 
> > >
> 
> > >
> 
> > >
> 
> > >
> 
> > > You pretty much can't use getModRefInfo right now unless you
> > > check
> 
> > > whether the instructions you are trying to get mod ref info about
> 
> > > are 0, 1, or 2 calls. Because calls don't have memorylocs, so you
> 
> > > can't just always call ::get on them.
> 
> > > You then have to call the right overloads yourself.
> 
> > > That seems wrong.
> 
> > > (I'll ignore the fact that you also have to check whether you are
> 
> > > holding a fence inst, since you can't get memorylocations for
> > > those
> 
> > > either, and ::get will just crash. )
> 
> > >
> 
> > >
> 
> > > Right now the number of overloads is also ... high, and while we
> 
> > > fixed the last immediate problem i remember (where random things
> 
> > > would get overloaded to ImmutableCallSite versions when they
> > > weren't
> 
> > > calls), staring at the number of getModRefInfo overloads, i'm not
> 
> > > sure i believe the way we have it structured makes sense.
> 
> > >
> 
> > >
> 
> > >
> 
> > >
> 
> > >
> 
> > >
> 
> > > On Wed, Oct 12, 2016 at 8:26 PM, Daniel Berlin <
> > > dberlin at dberlin.org
> 
> > > > wrote:
> 
> > >
> 
> > >
> 
> > >
> 
> > > You can get rid of UseInst too, MemoryLocOrCall will take a
> > > MemoryUse
> 
> > > directly :)
> 
> > >
> 
> > >
> 
> > >
> 
> > >
> 
> > >
> 
> > >
> 
> > > On Wed, Oct 12, 2016 at 8:20 PM, Sebastian Pop < sebpop at gmail.com
> > > >
> 
> > > wrote:
> 
> > >
> 
> > >
> 
> > > On Wed, Oct 12, 2016 at 10:05 PM, Daniel Berlin <
> > > dberlin at dberlin.org
> 
> > > > wrote:
> 
> > > > There is actually a much simpler fix you sholdl use.
> 
> > > >
> 
> > > > return instructionClobbersQuery(MD, MU MemoryLocOrCall(MU),
> > > > AA);
> 
> > > >
> 
> > > > You need to move that version of instructionClobbersQuery up,
> > > > but
> 
> > > > it was
> 
> > > > built to always do the right thing.
> 
> > > >
> 
> > > > Also note it will properly handle a case you are not: Fences do
> > > > not
> 
> > > > have
> 
> > > > memorylocations, and calling get on them will also crash.
> 
> > > >
> 
> > >
> 
> > > Right. I will commit the attached patch after testing.
> 
> > >
> 
> > > Thanks,
> 
> > > Sebastian
> 
> > >
> 
> > >
> 
> > >
> 

> > --
> 
> > Hal Finkel
> 
> > Lead, Compiler Technology and Programming Languages
> 
> > Leadership Computing Facility
> 
> > Argonne National Laboratory
> 

-- 

Hal Finkel 
Lead, Compiler Technology and Programming Languages 
Leadership Computing Facility 
Argonne National Laboratory 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161108/6d8c663d/attachment-0001.html>