[PATCH] D29047: Introduce a basic MemorySSA updater, that supports insertDef and insertUse operations.

[Daniel Berlin via Phabricator via llvm-commits]

dberlin marked 10 inline comments as done.
dberlin added inline comments.


================
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:741-742
 
+class MemorySSAUpdater {
+private:
+  MemorySSA *MSSA;
----------------
davide wrote:
> This is just my personal preference, so ignore it if you don't like, but, what about moving the updater to its own separate file?
I'll do this as a followup (otherwise it's going to completely screw up the comments here :P)


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:2475
+// This starts at the end of block
+MemoryAccess *MemorySSAUpdater::getPreviousDefFromEnd(BasicBlock *BB) {
+  auto *Defs = MSSA->getWritableBlockDefs(BB);
----------------
george.burgess.iv wrote:
> Naming nit: `MemorySSAUpdater::getLastDefIn`?
I went back and forth on what to call this one.
It used to be named getLastDef or something, but it's really not.
It's the same as the previous two functions, except it starts at the end of the block instead of a memory access.


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:2619
+    // Put any new phis on the fixup list, and process them
+    for (unsigned i = InsertedPHIs.size() - StartingPHISize;
+         i < InsertedPHIs.size(); ++i)
----------------
george.burgess.iv wrote:
> `FixupList.append(InsertedPhis.end() - StartingPHISize, InsertedPhis.end());`?
thanks, that's the function i was looking for.



================
Comment at: unittests/Transforms/Utils/MemorySSA.cpp:277
+TEST_F(MemorySSATest, MoveAStoreUpdater) {
+#if 0
+  // We create a diamond where there is a in the entry, a store on one side, and
----------------
george.burgess.iv wrote:
> Temporary (and below)?
Yes, the followup will add the move function


https://reviews.llvm.org/D29047