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

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added a comment.

Just a few nits. You can consider this a LGTM-with-nits if the concern I raised isn't actually an issue.



================
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:770
+//
+// For moving, just call moveBefore with the right arguments.
+//
----------------
nit: "just call moveBefore or moveAfter with"


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:2729
+    // Get the last memory access
+    const MemoryAccess *Last = &*(Accesses->rbegin());
+    BasicBlock::iterator BBPlace;
----------------
Please remove parens


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:2733
+    if (auto *MUD = dyn_cast<MemoryUseOrDef>(Last))
+      BBPlace = ++(MUD->getMemoryInst()->getIterator());
+    else
----------------
Please remove parens


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:2733
+    if (auto *MUD = dyn_cast<MemoryUseOrDef>(Last))
+      BBPlace = ++(MUD->getMemoryInst()->getIterator());
+    else
----------------
george.burgess.iv wrote:
> Please remove parens
I'm not super familiar with how BB Instruction ordering actually works, so this could totally be a misunderstanding on my part, but I think this might cause issues with operand dominance. Consider:

```
define void @foo() {
entry:
  ; 1 = MemoryDef
  %a = call i8* @bar()
  %b = bitcast i8* %a to i32*
  br i1 undef label %lbb, label %lbc

lbb:
  ; MemoryUse(1)
  %c = load i32, i32* %b
  br label %lbc

lbc:
  ret void
}
```

`updater.moveAfter(Use1, Def1)` will make %c dominate %b, since we call `TheLoad.moveBefore(EntryBlock, ++TheStore)`, and `++TheStore == TheBitcast`.

Same comment for blocks that only have `MemoryPhi`s, as well.


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:2736
+      BBPlace = BB->getFirstNonPHI()->getIterator();
+    What->getMemoryInst()->moveBefore(*BB, BBPlace);
+  } else {
----------------
Coming into this with no updater-related context outside of this patch, it's mildly surprising to me that MemorySSA's updater moves actual `Instruction`s around in BBs. It makes sense, but can we call that we do this out in the comments, please?


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:2761
+void MemorySSAUpdater::moveAfter(MemoryUseOrDef *What, MemoryUseOrDef *Where) {
+  moveTo(What, Where->getBlock(), ++(Where->getIterator()));
+}
----------------
Please remove the extra parens


https://reviews.llvm.org/D29047