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

[Hal Finkel via llvm-commits]

Hi Danny,

MemoryLocation does not apply to calls directly because a call can easily affect multiple locations. 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