[PATCH] D48396: [MemorySSA] Add APIs to MemoryPhis to delete incoming blocks/values, and an updater API to remove blocks.

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added a comment.

Thanks for this!



================
Comment at: include/llvm/Analysis/MemorySSA.h:566
 
+  void deleteIncoming(const unsigned I) {
+    unsigned E = getNumOperands();
----------------
nit: I don't think we encourage const on value params


================
Comment at: include/llvm/Analysis/MemorySSA.h:566
 
+  void deleteIncoming(const unsigned I) {
+    unsigned E = getNumOperands();
----------------
george.burgess.iv wrote:
> nit: I don't think we encourage const on value params
Naming nit: do you think it would be good to call out that these methods don't preserve the Phi's operand ordering? If so, please add either an `unordered` prefix/suffix to these.

(I've no issue with the swap-and-pop_back idiom; I just want it to be obvious at each callsite that the order might be changed)


================
Comment at: include/llvm/Analysis/MemorySSA.h:568
+    unsigned E = getNumOperands();
+    assert((I < E) && "Cannot remove out of bounds Phi entry.");
+    assert((E >= 2) && "Cannot only remove incoming values in MemoryPhis with "
----------------
Nit: The parens around `(I < E)` are unnecessary (and on line 569 + 585 + 597)


================
Comment at: include/llvm/Analysis/MemorySSA.h:570
+    assert((E >= 2) && "Cannot only remove incoming values in MemoryPhis with "
+                       "at least 2 values.");
+    setIncomingValue(I, getIncomingValue(E - 1));
----------------
Please add a comment explaining why (I'm assuming it's something like "otherwise, we should just be deleting the Phi outright")


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:501
+    TerminatorInst *TI = BB->getTerminator();
+    if (TI)
+      for (BasicBlock *Succ : TI->successors()) {
----------------
nit: `if (TerminatorInst *TI = BB->getTerminator())`.

Do we expect to see broken IR here, though? It's my impression that BBs should always have a terminator when they're in a complete-enough state.

If yes, please add a comment explaining how broken we expect our IR to be when this method is called. :)


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:514
+  for (BasicBlock *BB : DeadBlocks) {
+    while (MemorySSA::AccessList *Accesses =
+               MSSA->getWritableBlockAccesses(BB)) {
----------------
Do we need to look this up on every iteration?


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:519
+      if (!isa<MemoryUse>(MA) && !MA->use_empty()) {
+        if (MA->hasValueHandle())
+          ValueHandleBase::ValueIsDeleted(MA);
----------------
This is already done in `Value::~Value()`, which should be called by `MemorySSA::removeFromLists`. Does it need to be repeated here? (Especially given that we skip this extra check for Uses.)


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:524
+          Use &U = *MA->use_begin();
+          MemoryAccess *User = dyn_cast<MemoryAccess>(U.getUser());
+          // If user won't be deleted, it must be a MemoryPhi losing a value.
----------------
`cast<MemoryAccess>`


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:531
+          // LoE to clear MA's uses.
+          U.set(MSSA->getLiveOnEntryDef());
+        }
----------------
Can we set this to `nullptr` instead, like `Value::dropAllReferences` does? (I also think this could be simplified a fair amount if we swapped to `dropAllReferences` in the loop above + a deletion pass after that, but if that's inferior to this somehow, I'm happy to accept this approach. Value's dtor should cover the "User must be deleted too." assert for us)


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:538
+  }
+}
+
----------------
In the interest of getting rid of soon-to-be-dangling pointers, we should probably remove all of `DeadBlocks` from `MemorySSA::BlockNumberingValid`, too.

I think that would fit best in `MemorySSA::removeFromLists`, though


Repository:
  rL LLVM

https://reviews.llvm.org/D48396