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

Daniel Berlin dberlin at dberlin.org
Fri Feb 27 14:18:46 PST 2015


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.



>
> ================
> 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" :)


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


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


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

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.


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


>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150227/8c2533f0/attachment.html>


More information about the llvm-commits mailing list