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

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added inline comments.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:501
+    TerminatorInst *TI = BB->getTerminator();
+    if (TI)
+      for (BasicBlock *Succ : TI->successors()) {
----------------
george.burgess.iv wrote:
> 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. :)
TI should be null for empty blocks. I don't have a use case, but it seemed a reasonable safety check. I can remove it if you think it's unnecessary.
I extended the header comment with the amount of "broken-ness" to be expected :).


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:531
+          // LoE to clear MA's uses.
+          U.set(MSSA->getLiveOnEntryDef());
+        }
----------------
george.burgess.iv wrote:
> 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)
Added `dropAllReferences` above, only updating lists here.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:538
+  }
+}
+
----------------
george.burgess.iv wrote:
> 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
Added in `MemorySSA::removeFromLookups` where `BlockNumbering` is also updated.


Repository:
  rL LLVM

https://reviews.llvm.org/D48396