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

Tue Nov 8 13:33:00 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: Tuesday, November 8, 2016 3:23:54 PM
> Subject: Re: [PATCH] D25542: GVN-hoist: avoid calling
> MemoryLocation::get() on a call (PR30499)

> Sounds like we are of one mind, i'm on it.

> I'll probably start by making it functional, and then i'll see if i
> can split Mod into MayMod/MustMod, and Ref into MayRef/MustRef,
> which i presume would solve the other problem you mention (if not,
> yell now! :P)
Sounds great. We should split into May/Must/Partial to get all of the use cases. 

> Obviously, for instructions this works out just fine most of the
> time, and for calls, the default is min(may, must) for all
> MemoryLocations they could touch, unless you use the
> getModRefInfo(Call, MemoryLoc) interface we have.

Makes sense to me. 

Thanks again, 
Hal 

> On Tue, Nov 8, 2016 at 12:33 PM, Hal Finkel < hfinkel at anl.gov >
> wrote:

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

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

-- 

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/0795b515/attachment.html>