[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
Mon Apr 13 12:54:06 PDT 2015


On Mon, Apr 13, 2015 at 10:33 AM, Philip Reames
<listmail at philipreames.com> wrote:
> Ok, this review excludes MemorySSA.cpp.  I'll come back to that in a separate pass shortly.
>
> I see a bunch of small issues, but nothing so far that absolutely needs to be addressed before submission.  Given that a) this is an off by default change, and b) the author is a long established trusted committer, I have no problem with review comments being addresses as this evolves further in tree.
>

> When you do submit, please separate the AA changes into their own submission.  It should go in a few days before the memorySSA so that we can identify breakage in isolation.

Will do
>
> I would also strongly prefer to see more test cases included.  If nothing else, having a set of small examples for the semi obvious cases would make
> the usage of memory ssa much easier to understand.

I am happy to generate more testcases. I originally had some unit
tests that verified a bunch more (including the individual values),
then moved them to printing tests because that is apparently the
preference where possible.
In any case, I will add more tests for common/not so common cases i
have hit to make it obvious what is supposed to happen.

(I am also going to add API unittests for the the updating API anyway,
since it seems more difficult to do those as printing tests, since you
want to be able to verify immediate uses and such).


> ================
> Comment at: include/llvm/Analysis/AliasAnalysis.h:508
> @@ +507,3 @@
> +  /// memory used by a given call
> +  virtual bool instructionClobbersCall(Instruction *I,
> +                                       ImmutableCallSite Call);
> ----------------
> This seems like a subset of the getModRefInfo interface.  Is there a strong reason it needs to be phrased differently?

Do you mean you think this should be getModRefInfo(Instruction *,
ImmutableCallSite) (and return ModRefResult)

If so, SGTM, i'll update it.

If not, Hal suggested in the last review that this code get moved into
AA since AA has no way to ask whether a call and an instruction (which
may or may not also be a call) interact.


>
> ================
> Comment at: include/llvm/IR/Function.h:457
> @@ +456,3 @@
> +  /// AssemblyAnnotationWriter.
> +  void print(raw_ostream &OS, AssemblyAnnotationWriter *AAW = nullptr) const;
> +
> ----------------
> Again, submit separately please.
>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSA.h:15
> @@ +14,3 @@
> +// Memory SSA class builds an SSA form that links together memory
> +// access instructions such loads, stores, and clobbers (atomics,
> +// calls, etc), so they can be walked easily.  Additionally, it does a
> ----------------
> Given clobbers aren't instructions, please phrase this as:
> such as loads, stores, and calls, and atomics.
>
>
> Also, you *really* need to update this comment to include the bits about only mayalias accesses being on the immediate use list.  This is badly needed to prevent confusion.

Will do
>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSA.h:61
> @@ +60,3 @@
> +// Each def also has a list of uses
> +// Also note that it does not attempt any disambiguation, it is simply
> +// linking together the instructions.
> ----------------
> This comment is now explicitly wrong.
>
Yes.
Fixed


> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSA.h:100
> @@ +99,3 @@
> +  /// \brief Set the instruction that this MemoryUse represents.
> +  virtual void setMemoryInst(Instruction *MI) = 0;
> +
> ----------------
> Should this be part of the public interface?

No
Fixed
.
>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSA.h:117
> @@ +116,3 @@
> +
> +  unsigned use_size() const { return Uses.size(); }
> +  bool use_empty() const { return Uses.empty(); }
> ----------------
> Aren't all of these actually users not uses?  Not sure there's a real distinction for memory SSA, but this might be slightly confusing by analogy to normal def-use.  If there is no difference, maybe add a comment that clarifies that?

It is actually the users, i'll change the naming.

>
> One possible interesting case, what happens if you have a partially overlapping memcpy?  Is that a single user or two uses?

So, because of the way it is structured, it is a single user.
The same with phi's.  If you appear in a phi node multiple time, you
will still only get one use.

To give you some background, I looked at how Use.h worked, and it
seemed really tricky to get that working as a first pass, so i punted
on it and implemented Users.

We may want uses at some point, particularly because it makes
determining the path to the phi node easier for downwards-exposed
info.
(IE If you are a store, and ask for the users, given you may occur
multiple times in the same phi node, unless your store is in an
immediate predecessor, it seems tricky to determine which phi node
operand matches up with the store without walking the CFG)


>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSA.h:132
> @@ +131,3 @@
> +protected:
> +  friend class MemorySSA;
> +  friend class MemoryUse;
> ----------------
> Why do these need to be friends?  Shouldn't protected access be enough?  I *really* dislike having friend declarations here unless they're absolutely necessary.

Otherwise, you get this:

./lib/Transforms/Utils/MemorySSA.cpp:770:13: error: 'getID' is a
protected member of 'llvm::MemoryAccess'
    if (MA->getID() != 0)
            ^
../include/llvm/Transforms/Utils/MemorySSA.h:150:24: note: can only
access this member on an object of type 'llvm::MemoryPhi'
  virtual unsigned int getID() const { return 0; }

(and others), because you can't access them from the other classes.

So, for example, when the memoryphi tries to print operands, it gets
this error because they are not MemoryPhi's, so protected does not
give memoryphi class access to call it.


>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSA.h:148
> @@ +147,3 @@
> +  /// \brief Return true if \p Use is one of the uses of this memory access.
> +  bool findUse(MemoryAccess *Use) { return Uses.count(Use); }
> +
> ----------------
> "find" seems like the wrong name here.  Possible hasUse?

SGTM

>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSA.h:153
> @@ +152,3 @@
> +  /// \brief Used internally to give IDs to MemoryAccesses for printing
> +  virtual unsigned int getID() const { return 0; }
> +
> ----------------
> Should this possibly be a pure virtual function?  0 doesn't seem overly meaningful here.

I made it pure virtual.

>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSA.h:195
> @@ +194,3 @@
> +
> +  virtual void setDefiningAccess(MemoryAccess *DMA) final {
> +    if (DefiningAccess != DMA) {
> ----------------
> Again, does this need to be public?

No.


>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSA.h:227
> @@ +226,3 @@
> +/// \brief Represents a read-write access to memory, whether it is real, or a
> +/// clobber.
> +///
> ----------------
> Define clobber.  Preferrably using mayalias terminology.  References to AA docs are fine.

I updated this to be exact (and the same for MemoryUse).
MemoryUse now says:

/// In particular, the set of Instructions that will be represented by
/// MemoryUse's is exactly the set of Instructions for which
/// AliasAnalysis::getModRefInfo returns "Ref".

MemoryDef now says:
/// In particular, the set of Instructions that will be represented by
/// MemoryDef's is exactly the set of Instructions for which
/// AliasAnalysis::getModRefInfo returns "Mod" or "ModRef".


>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSA.h:260
> @@ +259,3 @@
> +/// These have the same semantic as regular phi nodes, with the exception that
> +/// only one phi will ever exist in a given basic block.
> +
> ----------------
> Is this still true with the mayalias changes?

Yes.  Ensuring that is why we do not disambiguate the RHS of MemoryDef's
(A disambiguated use will never cause the reaching *definition* to
change, because they are uses.
Disambiguated defs may cause multiple reaching definitions, and thus
we disallow them.
)


>  If so, how does that work?  Respond as a comment in code please.
>

Done.

(It now says:
/// These have the same semantic as regular phi nodes, with the exception that
/// only one phi will ever exist in a given basic block.
/// Guaranteeing one phi per block means guaranteeing there is only ever one
/// valid reaching MemoryDef/MemoryPHI along each path to the phi node.
/// This is ensured by not allowing disambiguation of the RHS of a MemoryDef or
/// a MemoryPHI's operands.
/// That is, given
/// if (a) {
///   store %a
///   store %b
/// }
/// it *must* be transformed into
/// if (a) {
///    1 = MemoryDef(liveOnEntry)
///    store %a
///    2 = MemoryDef(1)
///    store %b
/// }
/// and *not*
/// if (a) {
///    1 = MemoryDef(liveOnEntry)
///    store %a
///    2 = MemoryDef(liveOnEntry)
///    store %b
/// }
/// even if the two stores do not conflict otherwise, both 1 and 2 reach the
/// end of the branch, and if there are not two phi nodes, one will be
/// disconnected completely from the SSA graph below that point.
/// Because MemoryUse's do not generate new definitions, they do not
have this issue.



> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSA.h:269
> @@ +268,3 @@
> +
> +  virtual Instruction *getMemoryInst() const final { return nullptr; }
> +
> ----------------
> Should this be called on a phi?  Maybe llvm_unreachable?
Fixed to be llvm_unreachable


>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSA.h:292
> @@ +291,3 @@
> +  /// This function updates use lists.
> +  void setIncomingValue(unsigned int v, MemoryAccess *MA) {
> +    std::pair<BasicBlock *, MemoryAccess *> &Val = Args[v];
> ----------------
> Move definition to cpp please.
>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSA.h:326
> @@ +325,3 @@
> +  typedef SmallVector<std::pair<BasicBlock *, MemoryAccess *>, 8> ArgsType;
> +  typedef ArgsType::const_iterator const_arg_iterator;
> +  inline const_arg_iterator args_begin() const { return Args.begin(); }
> ----------------
> Args seems like a confusing name here.  What does the PHINode class call these?  Use that for consistency please.

I'll change it to operands.

>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSA.h:407
> @@ +406,3 @@
> +  /// \brief Remove a MemoryAccess from MemorySSA, including updating all
> +  // definitions and uses.
> +  void removeMemoryAccess(MemoryAccess *);
> ----------------
> When should this (and the next few routines) be used?

Whenever you change memory accesses.
I'll update the documentation a bit for each of these.

Currently the only user is the new MergedLoadStoreMotion and NewGVN.
>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSA.h:429
> @@ +428,3 @@
> +  /// instruction \p Replacer - this does not perform generic SSA updates, so it
> +  /// only works if the new access dominates the old accesses uses.
> +  ///
> ----------------
> Hopefully, this is checked in an assertion somewhere?

Yes, the functions it calls verify the domination.

>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSA.h:539
> @@ +538,3 @@
> +
> +/// \brief This is the generic walker interface for walkers of MemorySSA.
> +/// Walkers are used to be able to further disambiguate the def-use chains
> ----------------
> Can you expand this comment?  From the naming, it's not clear when I'd use one of these and why.
WIll do

>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSA.h:550
> @@ +549,3 @@
> +  /// will give you the nearest dominating clobbering MemoryAccess (by skipping
> +  /// non-aliasing def links).
> +  ///
> ----------------
> I'd suggest clarifying this as "by skipping any def which AA can prove does not alias the location(s) accessed by the instruction given"

Done. I also replaced the word "clobber" in most places with the word
"Mod", to match AA terminology.

>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSA.h:553
> @@ +552,3 @@
> +  /// Note that this will return a single access, and it must dominate the
> +  /// Instruction, so if an argument of a MemoryPhi node clobbers thenstruction,
> +  /// it will return the memoryphi node, *not* the argument.
> ----------------
> This comment is unclear?

This portion now reads:

 ///
  /// Note that this will return a single access, and it must dominate the
  /// Instruction, so if an operand of a MemoryPhi Mod's the instruction,
  /// this will return the MemoryPhi, not the operand.  This means that
  /// given:
  /// if (a) {
  ///   1 = MemoryDef(liveOnEntry)
  ///   store %a
  /// } else {
  ///   2 = MemoryDef(liveOnEntry)
  ///    store %b
  /// }
  /// 3 = MemoryPhi(2, 1)
  /// MemoryUse(3)
  /// load %a
  ///
  /// calling this API on load(%a) will return the MemoryPhi, not the MemoryDef
  /// in the if (a) branch.


>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSA.h:562
> @@ +561,3 @@
> +  // the nearest clobbering access of all of phi arguments, instead of the phi.
> +  // virtual MemoryAccessSet getClobberingMemoryAccesses(const Instruction *) =
> +  // 0;
> ----------------
> Dead code, please remove!

Done.

>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSA.h:571
> @@ +570,3 @@
> +/// simply returns the links as they were constructed by the builder.
> +class DoNothingMemorySSAWalker final : public MemorySSAWalker {
> +public:
> ----------------
> I'm confused, am I allowed to call getClobberingMemoryAccess on a DoNothingMemorySSAWalker?  What are the semantics here?

Yes, it gives you back the same thing you passed it.

>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSA.h:576
> @@ +575,3 @@
> +
> +/// \brief A MemorySSAWalker that does real AA walks and caching of lookups.
> +class CachingMemorySSAWalker final : public MemorySSAWalker {
> ----------------
> "real"?  As opposed to?

Doing nothing, but i'll change it.

>
> ================
> Comment at: lib/Analysis/AliasAnalysis.cpp:351
> @@ -332,3 +350,3 @@
>    // or write the specified memory.
> -  if (!alias(getLocation(L), Loc))
> +  if (Loc.Ptr && !alias(getLocation(L), Loc))
>      return NoModRef;
> ----------------
> These *need* to submitted separately with test cases if possible.

So, you can't make it crash in the current callers, but you can with MemorySSA.

In this case, our goal is just to determine whether this is an
instruction that reads/writes memory, without regard to a specific
location.

But, if you call getModRefInfo with an empty Loc.Ptr, it will crash on
the instructions I updated.
How do you get an empty Loc.Ptr?

By  passing a default constructed AliasAnalysis::Location to them.

This (passing default constructed AliasAnalysis::Location) does not
happen in the other caller (MemoryDependenceAnalysis), because it
specifically avoids it through other tricks.

I used to handle this in MemorySSA by filling in a fake Loc.Ptr. Hal
suggested I just update AliasAnalysis to not crash instead, which will
let use remove the need for tricks at all (after all, nothing in the
API makes it invalid to pass default constructed locations to these
functions))

In any case, i will submit it separately, but note that every testcase
that uses memoryssa would trigger this bug otherwise (since memoryssa
will call getModRefInfo on every instructions, and will crash on the
load/stores as a result)


>
> http://reviews.llvm.org/D7864
>
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/
>
>




More information about the llvm-commits mailing list