[PATCH] D17157: Add the beginnings of an update API for preserving MemorySSA

[George Burgess IV via llvm-commits]

george.burgess.iv added a comment.

Woohoo! A few comments for you (mostly nits)


================
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:518
@@ -517,1 +517,3 @@
   }
+  /// \brief Remove a MemoryAccess from MemorySSA, including updating all
+  /// definitions and uses.
----------------
Nit: Please add a newline before this

================
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:679
@@ -666,1 +678,3 @@
+  /// the walker it uses or returns.
+  virtual void invalidateInfo(MemoryAccess *){};
 
----------------
Nit: space after `)`, useless `;` at the end.

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:492
@@ +491,3 @@
+  // Re-point the uses at our defining access
+  MA->replaceAllUsesWith(NewDefTarget);
+
----------------
It seems that if `MA` is a Phi with no Uses (e.g. they were removed, ...), `NewDefTarget` may be null here. Is this intentional/possible?

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:815
@@ +814,3 @@
+
+  if (!isa<MemoryPhi>(MA)) {
+    MemoryUseOrDef *MUD = cast<MemoryUseOrDef>(MA);
----------------
Nit: Can we use `if (auto *MUD = dyn_cast<MemoryUseOrDef>(MA))` instead?

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:818
@@ +817,3 @@
+    Instruction *I = MUD->getMemoryInst();
+    if (bool(ImmutableCallSite(I))) {
+      IsCall = true;
----------------
Nit: `IsCall = bool(ImmutableCallSite(I))`


http://reviews.llvm.org/D17157