[PATCH] This patch introduces MemorySSA, a virtual SSA form for memory.Details on what it looks like are in MemorySSA.h

Hal Finkel hfinkel at anl.gov
Fri Feb 27 14:48:32 PST 2015


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

> From: "Daniel Berlin" <dberlin at dberlin.org>
> To: reviews+D7864+public+020798531ef64731 at reviews.llvm.org
> Cc: "Hal Finkel" <hfinkel at anl.gov>, "Nick Lewycky"
> <nlewycky at google.com>, "James Molloy" <james.molloy at arm.com>,
> "Philip Reames" <listmail at philipreames.com>, "Sanjoy Das"
> <sanjoy at playingwithpointers.com>, "Commit Messages and Patches for
> LLVM" <llvm-commits at cs.uiuc.edu>
> Sent: Friday, February 27, 2015 4:18:46 PM
> Subject: Re: [PATCH] This patch introduces MemorySSA, a virtual SSA
> form for memory.Details on what it looks like are in MemorySSA.h

> On Fri, Feb 27, 2015 at 1:18 PM, hfinkel at anl.gov < hfinkel at anl.gov >
> wrote:

> > ================
> 
> > Comment at: lib/Transforms/Utils/MemorySSA.cpp:86
> 
> > @@ +85,3 @@
> 
> > +// This is mostly ripped from MemoryDependenceAnalysis, updated to
> 
> > +// handle call instructions properly
> 
> > +
> 
> > ----------------
> 
> > What exactly does "updated to handle call instructions properly"
> > mean? Also, this looks like it should be a member function of AA.
> > Can you please add it there (then we can update MDA to use it
> > too?).
> 

> It means MDA's version would crash on call instructions :)

> Happy to make it part of AA and update both.
I think that's a good idea. Having several only-slightly-different utility functions for this scattered about does not seem optimal. 

> > ================
> 
> > Comment at: lib/Transforms/Utils/MemorySSA.cpp:100
> 
> > @@ +99,3 @@
> 
> > + else if (auto *I = dyn_cast<CallInst>(Inst))
> 
> > + return AliasAnalysis::Location(I);
> 
> > + else if (auto *I = dyn_cast<InvokeInst>(Inst))
> 
> > ----------------
> 
> > Part of my confusion is this: What does the call's return value
> > have
> > to do with the memory location(s) accessed by the call?
> 

> Nothing. But BasicAA crashes on Calls/Invokes if you call
> getModRefInfo and Loc.Ptr is null. MDA sets is to the call
> instruction, so we do the same.

> (Note: getModRefInfo ends up using the right call down the line, it
> creates an immutablecallsite out of it, and uses that vs actual
> location)

> You are more than welcome to say "we should go and fix this" :)
Good, we should go and fix this ;) 

> > We do have these in AA:
> 

> > static Location getLocationForSource(const MemTransferInst *MTI);
> 
> > static Location getLocationForDest(const MemIntrinsic *MI);
> 

> > and I imagine those would be good to use here.
> 

> > But for generic calls, we don't have any facility to get a
> > location,
> > or list of locations, touched by the call. We can do call-site vs.
> > call-site queries, and call-site vs. Location queries, however.
> 

> It is actually doing a call-site query, the one place this got used
> was essentially to tell whether a call was readonly/had no memory
> effects.

> > ================
> 
> > Comment at: lib/Transforms/Utils/MemorySSA.cpp:136
> 
> > @@ +135,3 @@
> 
> > + // 1. One argument dominates the other, and the other's version
> > is
> > a
> 
> > + // "false" one.
> 
> > + // 2. All of the arguments are false version numbers that
> > eventually
> 
> > ----------------
> 
> > Can you please define here what a "false" version is?
> > (non-aliasing?)
> 

> Will do (it's non-aliasing)

> > ================
> 
> > Comment at: lib/Transforms/Utils/MemorySSA.cpp:241
> 
> > @@ +240,3 @@
> 
> > + break;
> 
> > + } else if (AA->alias(getLocationForAA(AA, DefMemoryInst), Loc) !=
> 
> > + AliasAnalysis::NoAlias)
> 
> > ----------------
> 
> > I'm likely missing something here, but why don't you just call
> > AA->getModRefInfo for all types of instructions? The overload for
> > Instruction* handles all of the various types:
> 

> It doesn't actually work properly on some types of Loc, despite it
> trying to dispatch everything :)

> Happy to fix it :)
Yes, please do :) 

> > /// getModRefInfo - Return information about whether or not an
> > instruction may
> 
> > /// read or write the specified memory location. An instruction
> 
> > /// that doesn't read or write memory may be trivially LICM'd for
> > example.
> 
> > ModRefResult getModRefInfo(const Instruction *I,
> 
> > const Location &Loc) {
> 
> > switch (I->getOpcode()) {
> 
> > case Instruction::VAArg: return getModRefInfo((const VAArgInst*)I,
> > Loc);
> 
> > case Instruction::Load: return getModRefInfo((const LoadInst*)I,
> > Loc);
> 
> > case Instruction::Store: return getModRefInfo((const StoreInst*)I,
> > Loc);
> 
> > case Instruction::Fence: return getModRefInfo((const FenceInst*)I,
> > Loc);
> 
> > case Instruction::AtomicCmpXchg:
> 
> > return getModRefInfo((const AtomicCmpXchgInst*)I, Loc);
> 
> > case Instruction::AtomicRMW:
> 
> > return getModRefInfo((const AtomicRMWInst*)I, Loc);
> 
> > case Instruction::Call: return getModRefInfo((const CallInst*)I,
> > Loc);
> 
> > case Instruction::Invoke: return getModRefInfo((const
> > InvokeInst*)I,Loc);
> 
> > default: return NoModRef;
> 
> > }
> 
> > }
> 

> > ================
> 
> > Comment at: lib/Transforms/Utils/MemorySSA.cpp:269
> 
> > @@ +268,3 @@
> 
> > + ++NumClobberCacheHits;
> 
> > + DEBUG(dbgs() << "Cached Memory SSA version for " << (uintptr_t)I
> > <<
> > " is ");
> 
> > + DEBUG(dbgs() << (uintptr_t)CCV->second);
> 
> > ----------------
> 
> > Why are you printing the instruction addresses here? Would it be
> > more
> > useful to print *I?
> 

> So, you can't really print anything sane anymore other than that
> since I removed the otherwise-unused defVersion field in a previous
> patch. If you look a few versions back in the patch, it used to
> print

> "<version> = MemoryDef(<version>)" if you printed *I, and the
> operator override would print the version here, so you'd get

> <version> used to come from the defVersion member of the structure.

> defVersion is superfluous, however, since they are really linked the
> same way LLVM instructions/values are linked -> memory pointer to
> memory pointer.
> It was suggested i remove it, so i did. This means you don't know
> what <version> should be, outside of a memory address, unless you
> make it up.

> The only way to make a "nice" printout now is to do what the
> annotation writer does, which is assign each memoryaccess* unique
> numbers and use them consistently (like slottracker does)
Okay. 

> > ================
> 
> > Comment at: lib/Transforms/Utils/MemorySSA.cpp:651
> 
> > @@ +650,3 @@
> 
> > + Loc.Ptr = BI;
> 
> > + AliasAnalysis::ModRefResult ModRef = AA->getModRefInfo(BI, Loc);
> 
> > + if (ModRef & AliasAnalysis::Mod)
> 
> > ----------------
> 
> > You're testing against an empty location?
> 
> Yes. The goal is to figure out whether the instruction can have
> memory side-effects. If you look at AliasAnalysis.cpp, that is, in
> fact, what it does with an empty Loc :)

> Better suggestions welcome, but duplicating all the functionality it
> has in there to know things about load ordering flags, etc, seemed a
> bad idea.
I don't want to duplicate that functionality, but I also don't want to work-around a insufficiently-expressive AA interface. And regardless, I certainly don't want to embed these kinds of AA implementation details in other passes. 

> Note that this crashes on call if Loc.ptr is null, but does not crash
> on any other type of instruction if loc.ptr is null.

> > I don't think this is what you want, how about using this:
> 

> > /// getModRefBehavior - Return the behavior when calling the given
> > call site.
> 
> > virtual ModRefBehavior getModRefBehavior(ImmutableCallSite CS);
> 

> This won't work for anything but calls.
> This loop is basically trying to figure out where to place defs. it
> needs to place defs anywhere something can actually write to memory,
> no matter what the instruction type, etc.

> It is using getModRefInfo to get this behavior.
Sure, but my point was that you're already special-casing call sites, so why not just use getModRefBehavior for these cases. Perhaps better yet, we can fix the AA implementation to deal with an empty Location. 

> > ================
> 
> > Comment at: lib/Transforms/Utils/MemorySSA.cpp:688
> 
> > @@ +687,3 @@
> 
> > + // arguments or users of globals. We do not actually insert it in
> > to
> 
> > + // the IR.
> 
> > + BasicBlock &StartingPoint = F.getEntryBlock();
> 
> > ----------------
> 
> > But you don't really insert anything into the IR proper. I'd
> > rephrase
> > this.
> 

> will do

Thanks again, 
Hal 

-- 

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/20150227/d70d5b08/attachment.html>


More information about the llvm-commits mailing list