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

[George Burgess IV via llvm-commits]

george.burgess.iv added a comment.

Thanks for the LGTM, Danny.

First wave of responses...


================
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:133
@@ +132,3 @@
+  const_memoryaccess_def_iterator defs_end() const;
+
+protected:
----------------
Because it breaks code like (ripped from `MemoryDef::print(raw_ostream &)`):

```
MemoryAccess *UO = getDefiningAccess();
if (UO && UO->getID()) // Breaks
  // ...
```

...Because `protected` doesn't let you call `protected` methods on other `MemoryAccess`es. You can call them on yourself, or you can call them on other `MemoryDef`s. That's all.

Do you think it would be better to make `getID()` public, or to keep the friend declarations? I'm happy with either.

================
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:151
@@ +150,3 @@
+  void operator=(const MemoryAccess &);
+  BasicBlock *Block;
+};
----------------
It wasn't entirely an isa relationship. It's now refactored to be more clear.

================
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:293
@@ +292,3 @@
+private:
+  const unsigned ID;
+};
----------------
Definition of `MemoryPhi::setIncomingValue` is now substantially smaller. Leaving in header, unless you'd still like to see it moved.

================
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:408
@@ +407,3 @@
+  void addIncoming(MemoryAccess *V, BasicBlock *BB) {
+    if (getNumOperands() == ReservedSpace)
+      growOperands(); // Get more space!
----------------
Update API has been removed for now.

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:80
@@ +79,3 @@
+      OS << "; " << *MA << "\n";
+  }
+};
----------------
Comment is on code that seems to have been removed.

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:87
@@ +86,3 @@
+  DomTreeNode *DTN;
+  DomTreeNode::const_iterator ChildIt;
+  MemoryAccess *IncomingVal;
----------------
Comment is on code that seems to have been removed.

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:152
@@ +151,3 @@
+void MemorySSA::renamePass(DomTreeNode *Root, MemoryAccess *IncomingVal,
+                           SmallPtrSet<BasicBlock *, 16> &Visited) {
+  SmallVector<RenamePassData, 32> WorkStack;
----------------
Comment is on code that seems to have been removed.

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:621
@@ +620,3 @@
+
+char MemorySSALazy::ID = 0;
+
----------------
Now only exists in one place AFAICT.

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:963
@@ +962,2 @@
+}
+}
----------------
Nuked `possiblyAffectedBy` from orbit; it's an optimization that can be readded later.

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:963
@@ +962,2 @@
+}
+}
----------------
george.burgess.iv wrote:
> Nuked `possiblyAffectedBy` from orbit; it's an optimization that can be readded later.
Do you still hold this opinion? (Potentially dubious is now in `CachingMemorySSAWalker::instructionClobbersQuery`)

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:963
@@ +962,2 @@
+}
+}
----------------
george.burgess.iv wrote:
> george.burgess.iv wrote:
> > Nuked `possiblyAffectedBy` from orbit; it's an optimization that can be readded later.
> Do you still hold this opinion? (Potentially dubious is now in `CachingMemorySSAWalker::instructionClobbersQuery`)
Marked with a TODO so we can look into that when we have users of MemorySSA/a more complete API/...

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:963
@@ +962,2 @@
+}
+}
----------------
george.burgess.iv wrote:
> george.burgess.iv wrote:
> > george.burgess.iv wrote:
> > > Nuked `possiblyAffectedBy` from orbit; it's an optimization that can be readded later.
> > Do you still hold this opinion? (Potentially dubious is now in `CachingMemorySSAWalker::instructionClobbersQuery`)
> Marked with a TODO so we can look into that when we have users of MemorySSA/a more complete API/...
Seems old; we now use DFS. Marking as done, but added a test case of your second example.

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:963
@@ +962,2 @@
+}
+}
----------------
george.burgess.iv wrote:
> george.burgess.iv wrote:
> > george.burgess.iv wrote:
> > > george.burgess.iv wrote:
> > > > Nuked `possiblyAffectedBy` from orbit; it's an optimization that can be readded later.
> > > Do you still hold this opinion? (Potentially dubious is now in `CachingMemorySSAWalker::instructionClobbersQuery`)
> > Marked with a TODO so we can look into that when we have users of MemorySSA/a more complete API/...
> Seems old; we now use DFS. Marking as done, but added a test case of your second example.
I think it's better to get MemorySSA in and work on little optimizations like this after that. Removed the invariant check (but kept the test as an XFAIL), to be readded in the near future as a set of enhancements.


http://reviews.llvm.org/D7864