[PATCH] This patch introduces MemorySSA, a virtual SSA form for memory.Details on what it looks like are in MemorySSA.h
Philip Reames
listmail at philipreames.com
Mon Apr 13 10:33:09 PDT 2015
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.
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.
================
Comment at: include/llvm/Analysis/AliasAnalysis.h:148
@@ -147,1 +147,3 @@
static Location getLocationForDest(const MemIntrinsic *MI);
+ Location getLocation(const Instruction *Inst) {
+ if (auto *I = dyn_cast<LoadInst>(Inst))
----------------
This can and should go in separately.
================
Comment at: include/llvm/Analysis/AliasAnalysis.h:371
@@ +370,3 @@
+ /// specific location)
+ ModRefResult getModRefInfo(const Instruction *I) {
+ if (isa<InvokeInst>(I) || isa<CallInst>(I)) {
----------------
Same as above.
================
Comment at: include/llvm/Analysis/AliasAnalysis.h:374
@@ +373,3 @@
+ auto MRB = getModRefBehavior(I);
+ if (MRB & ModRef)
+ return ModRef;
----------------
I might make this a switch so that all the cases are obviously covered and checked by the compiler.
================
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?
================
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.
================
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.
================
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?
================
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?
One possible interesting case, what happens if you have a partially overlapping memcpy? Is that a single user or two uses?
================
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.
================
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?
================
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.
================
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?
================
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.
================
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? If so, how does that work? Respond as a comment in code please.
================
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?
================
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.
================
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?
================
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?
================
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.
================
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"
================
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?
================
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!
================
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?
================
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?
================
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.
http://reviews.llvm.org/D7864
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
More information about the llvm-commits
mailing list