[PATCH] D21463: Add MemoryAccess creation and PHI creation APIs to MemorySSA

[George Burgess IV via llvm-commits]

george.burgess.iv added a comment.

Looks good overall -- just a few nits.


================
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:553
@@ +552,3 @@
+  /// It will return the new MemoryAccess.
+  MemoryAccess *createMemoryAccess(Instruction *I, MemoryAccess *Definition,
+                                   const BasicBlock *BB, InsertionPlace Point);
----------------
`createMemoryAccessInBB`, maybe? Naming is hard.

================
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:557
@@ +556,3 @@
+  /// MemoryAccess.
+  //
+  /// Returns the new MemoryAccess.
----------------
///

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:589
@@ +588,3 @@
+/// order and existence of memory affecting instructions.
+
+void MemorySSA::verifyOrdering(Function &F) const {
----------------
Please remove this newline

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:595
@@ +594,3 @@
+  for (BasicBlock &B : F) {
+    SmallVector<MemoryAccess *, 32> ActualAccesses;
+    const AccessListType *AL = getBlockAccesses(&B);
----------------
Bring this out of the loop + clear on each iteration, please.

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:602
@@ +601,3 @@
+      MemoryAccess *MA = getMemoryAccess(&I);
+      assert((!MA || AL != nullptr) && "We have memory affecting instructions "
+                                       "in this block but they are not in the "
----------------
Varying compare-against-null styles?


http://reviews.llvm.org/D21463