[PATCH] D48897: [MemorySSA] Add APIs to move memory accesses between blocks, following CFG changes.

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added a comment.

Thanks for the patch!



================
Comment at: include/llvm/Analysis/MemorySSAUpdater.h:107
+                                Instruction *Start);
+  /// `From` block was merged into `To`. All instructiones were moved and
+  /// `From` is an empty block with successor edges; `From` is about to be
----------------
s/instructiones/instructions/


================
Comment at: include/llvm/Analysis/MemorySSAUpdater.h:123
+                               Instruction *Start);
+  /// BasicBlock Old was split and now has a new immediate predecessor (New),
+  /// whose predecessors are Old's new predecessors. If a phi existed in Old,
----------------
Were any instructions moved as part of the split, or did we just insert New between Old and all of its predecessors? In the latter case, I'd nitpick this to something more like "BasicBlock Old had New, an empty BasicBlock, added directly before it, making Old's only predecessor New. Move Old's Phi, if present, to New."


================
Comment at: include/llvm/Analysis/MemorySSAUpdater.h:203
+  void moveAllAccesses(BasicBlock *From, BasicBlock *To, Instruction *Start,
+                       MemorySSA::InsertionPlace Where);
   MemoryAccess *getPreviousDef(MemoryAccess *);
----------------
I'm assuming we have plans to make `Where != End` in future patches? (If so, that's OK; I'm just checking, since it looks like we're only passing in `End` right now)


================
Comment at: lib/Analysis/MemorySSA.cpp:1483
+    ValueToMemoryAccess.erase(What->getBlock());
+    ValueToMemoryAccess[BB] = What;
+  }
----------------
Would it also be good to assert that `BB` doesn't currently have a Phi?

(We could get that sort of implicitly if we used `insert(...).second` instead of `operator[]` here)


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:438
+
+  if (MemorySSA::AccessList *Accs = MSSA->getWritableBlockAccesses(From)) {
+    MemoryAccess *FirstInNew = nullptr;
----------------
Prefer [early exits](https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code) if the code inside the `if` is long like this, please. (And below, at `if (FirstInNew)`)


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:441
+    BasicBlock::iterator StartSearch = Start->getIterator();
+    while (StartSearch != To->end()) {
+      FirstInNew = MSSA->getMemoryAccess(&*StartSearch);
----------------
`for (Instruction &I : make_range(Start->getIterator(), To->end()) {`?


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:448
+    if (FirstInNew) {
+      auto *MUD = dyn_cast_or_null<MemoryUseOrDef>(FirstInNew);
+      auto NextIt = ++MUD->getIterator();
----------------
s/dyn_cast_or_null/cast/, since we're dereferencing it on the next line.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:452
+          (NextIt == Accs->end()) ? nullptr
+                                  : dyn_cast_or_null<MemoryUseOrDef>(&*NextIt);
+      MSSA->moveTo(MUD, To, Where);
----------------
`cast`, please.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:456
+
+      while (NextMUD) {
+        MUD = NextMUD;
----------------
Is it possible to fold some of the above code into this loop without losing much in the way of clarity? It looks really similar to this loop body to me.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:462
+                      ? nullptr
+                      : dyn_cast_or_null<MemoryUseOrDef>(&*NextIt);
+        MSSA->moveTo(MUD, To, InsertionPlace);
----------------
Same nit about `cast`.




================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:476
+  moveAllAccesses(From, To, Start, MemorySSA::End);
+  for (succ_iterator Si = succ_begin(To), Se = succ_end(To); Si != Se; ++Si)
+    if (MemoryPhi *MPhi = MSSA->getMemoryAccess(*Si))
----------------
Would `for (BasicBlock *Succ : successors(To))` work? (and below)


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:496
+  MemoryPhi *Phi = MSSA->getMemoryAccess(Old);
+  if (!Phi)
+    return;
----------------
Micro style nit:

```
if (MemoryPhi *Phi = ...)
  MSSA->moveTo(...)
```


Repository:
  rL LLVM

https://reviews.llvm.org/D48897