[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/MemorySSA.cpp:1576
   BlockNumbering.erase(MA);
+  BlockNumberingValid.erase(MA->getBlock());
   if (auto *MUD = dyn_cast<MemoryUseOrDef>(MA))
----------------
george.burgess.iv wrote:
> 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.
You're right :). Moved to `removeFromLists`.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:501
+    TerminatorInst *TI = BB->getTerminator();
+    if (TI)
+      for (BasicBlock *Succ : TI->successors()) {
----------------
george.burgess.iv wrote:
> 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.
Ack, adding an assert. If I find a usecase later, I'll change it to a check.


Repository:
  rL LLVM

https://reviews.llvm.org/D48396