[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 inline comments.


================
Comment at: include/llvm/Analysis/MemorySSAUpdater.h:153
+  /// they occur in phis that will simply lose an incoming value.
+  /// Deleted blocks still have successor info, but their predecessor edged and
+  /// Phi nodes may already be updated. Instructions in DeadBlocks should be
----------------
Did you mean "predecessor edges"?


================
Comment at: lib/Analysis/MemorySSA.cpp:1576
   BlockNumbering.erase(MA);
+  BlockNumberingValid.erase(MA->getBlock());
   if (auto *MUD = dyn_cast<MemoryUseOrDef>(MA))
----------------
The numbering is still valid even if we remove MA, though :)

We don't really care if the numbering has gaps. We only care that the block numbers increase as you iterate through an AccessList.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:501
+    TerminatorInst *TI = BB->getTerminator();
+    if (TI)
+      for (BasicBlock *Succ : TI->successors()) {
----------------
asbirlea wrote:
> 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 :).
Personally, I have a slight preference for asserting that the IR is well-formed until we know that we can't, but if you'd prefer to have a check here, I'm OK with that.


Repository:
  rL LLVM

https://reviews.llvm.org/D48396