[PATCH] D49156: [MemorySSA] Add API to update MemoryPhis, following CFG changes.

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added inline comments.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:499
+    BasicBlock *Old, BasicBlock *New, ArrayRef<BasicBlock *> Preds) {
+  assert(MSSA->getWritableBlockAccesses(New) == nullptr &&
+         "Access list should be null for a new block.");
----------------
nit: Please just use `!` instead of `== nullptr`, for consistency


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:504
+    return;
+  if (pred_size(Old) == 1 && pred_size(New) == Preds.size())
+    MSSA->moveTo(Phi, New, MemorySSA::Beginning);
----------------
Doesn't `pred_size(Old) == 1` imply that `pred_size(New) == Preds.size()` must be `true`? Or is there a case I'm missing?


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:507
+  else {
+    assert(Preds.size() > 0 && "Must be moving at least one predecessor to the "
+                               "new immediate predecessor.");
----------------
nit: s/`Preds.size() > 0`/`!Preds.empty()`/


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:517
+        Phi->unorderedDeleteIncoming(Idx);
+        Idx = Phi->getBasicBlockIndex(Pred);
+      }
----------------
Looks like we could do this in O(`Preds.size() + Phi->getNumOperands()`) time if we instead used a `SmallPtrSet` for `Preds` and did a single walk over the operands list, querying the `SmallPtrSet` for whether $current_operand is something we could remove.

If we made a general utility for Phis similar in spirit to `llvm::erase_if` for this, looks like we could even make `unorderedDeleteIncomingBlock` and `unorderedDeleteIncomingValue` tiny wrappers around that.

It would arguably be mildly awkward to have the `addIncoming` side-effect from the lambda there, but I'd imagine it would be cleaner on the whole. WDYT?


Repository:
  rL LLVM

https://reviews.llvm.org/D49156