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

George Burgess IV via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jul 12 15:30:17 PDT 2018


george.burgess.iv added inline comments.


================
Comment at: include/llvm/Analysis/MemorySSA.h:581
 
-  // After deleting incoming block BB, the incoming blocks order may be changed.
-  void unorderedDeleteIncomingBlock(const BasicBlock *BB) {
+  // After deleting entried that satisfy Pred, remaining entries may have
+  // changed order.
----------------
s/entried/entries


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:517
+        Phi->unorderedDeleteIncoming(Idx);
+        Idx = Phi->getBasicBlockIndex(Pred);
+      }
----------------
asbirlea wrote:
> george.burgess.iv wrote:
> > 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?
> I think I like this more than the alternatives I thought about. So here goes :)
Did making a `SmallPtrSet` of Preds not work?


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:516
+            if (Pred == B) {
+              NewPhi->addIncoming(Phi->getIncomingValueForBlock(Pred), Pred);
+              return true;
----------------
Can we replace `Phi->getIncomingValueForBlock(B)` with the `MemoryAccess *` we're passed in (if we make it non-`const`)?


Repository:
  rL LLVM

https://reviews.llvm.org/D49156





More information about the llvm-commits mailing list