[PATCH] D30154: MemorySSA: Add support for renaming uses in the updater.

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added a comment.

handful o'nits to supplement piotr's comments, then this LGTM.

thanks!



================
Comment at: include/llvm/Transforms/Utils/MemorySSAUpdater.h:66
   MemorySSAUpdater(MemorySSA *MSSA) : MSSA(MSSA) {}
-  void insertDef(MemoryDef *Def);
+  // Insert a definition into the MemorySSA IR.  RenameUses will rename any use
+  // below the new def block (and any inserted phis).  RenameUses should be set
----------------
should this be ///?


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1188
+      // We already visited this during our renaming, which can happen when
+      // being asked to rename multipleblocks. Figure out the incoming val,
+      // which is the last def
----------------
missing space between "multiple blocks"


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1191
+      if (!Visited.insert(BB).second) {
+        auto *BlockDefs = getWritableBlockDefs(BB);
+        // Incoming value can only change if there is a block def, and in that
----------------
please sink this into the if condition


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1195
+        if (BlockDefs)
+          IncomingVal = &*(BlockDefs->rbegin());
+      } else
----------------
please remove redundant parens


================
Comment at: lib/Transforms/Utils/MemorySSAUpdater.cpp:299
+     // incoming value.
+    if (isa<MemoryDef>(FirstDef))
+      FirstDef = cast<MemoryDef>(FirstDef)->getDefiningAccess();
----------------
please do `if (auto ... = dyn_cast<MemoryDef>(FirstDef))` instead


================
Comment at: unittests/Transforms/Utils/MemorySSA.cpp:193
   MSSA.verifyMemorySSA();
+  MSSA.dump();
 }
----------------
davide wrote:
> IIRC `dump()` is supposed to be used only in a debugger and `print()` should be used instead, but I might be wrong.
please remove


https://reviews.llvm.org/D30154