[PATCH] D45299: API to update MemorySSA for cloned blocks.

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added a comment.

(FWIW, Alina and I chatted about this offline. She has a few changes queued up; I'm adding my nits on unchanged bits now, and will do a thorough code review once the new patch is posted)



================
Comment at: include/llvm/Analysis/MemorySSAUpdater.h:105
+  /// `To` is newly created BB, so empty of MemorySSA::MemoryAccesses.
+  /// |------|        |------|
+  /// | From |        | From |
----------------
I like the ascii art :)


================
Comment at: include/llvm/Analysis/MemorySSAUpdater.h:131
+  /// Due to block cloning or block splitting, BB was added additional
+  /// predecessors beside the previously known single predecessor BB_Pred.
+  /// Update or insert Phi nodes to make MemorySSA valid again.
----------------
Nit: "was added additional predecessors" should probably be worded something like "BB had additional predecessors added"


================
Comment at: include/llvm/Analysis/MemorySSAUpdater.h:93
+  /// the loop exit blocks and a 1:1 mapping of all blocks and instructions
+  /// cloned. This involves duplicating all defs and uses in the cloned blocks
+  /// and updating phi nodes in exit block successors.
----------------
nit: all defs, uses, and phis


================
Comment at: include/llvm/Analysis/MemorySSAUpdater.h:131
+  /// `From` block was merged into `To`.
+  /// Move all accesses from `From` to `To` starting at instruction `Start`.
+  /// `To` has a single successor and `From` has a single predecessor.
----------------
Nit: is `Start` the instruction that was previously the last instruction in `To`?


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:508
+
+void MemorySSAUpdater::insertPhisIfNeeded(BasicBlock *BB, BasicBlock *P1,
+                                          DominatorTree *DT) {
----------------
asbirlea wrote:
> george.burgess.iv wrote:
> > Insert phis if needed because... what happened? :)
> > 
> > I tried to read the loop code, but got a few layers deep in unfamiliar jargon and gave up.
> Update to `updatePhisWhenAddingBBPredecessors`.
> Kinda long, but at least more insightful? :-/
Much better. :)

I'm inclined to think `updatePhisForNewBBPredecessors` might be a bit more clear, but I'm also fine with this if you disagree.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:542
+
+  SmallPtrSet<BasicBlock *, 8> Seen;
+  SmallVector<BasicBlock *, 16> WorklistBB;
----------------
asbirlea wrote:
> george.burgess.iv wrote:
> > General note: I think that `Seen` + `Worklist` combos can be replaced with a `SmallSetVector`. Not something we need to worry about at this very second, but...
> Can you elaborate on this one? The Seen and Worklist structures keep different data, but perhaps I'm missing something here.
> Btw, I used the same pattern I saw in fixupDefs, for some consistency.
...Do they? AFAICT, conceptually, this code does a BFS across nodes in our graph of BBs, and `WorklistBB` is the queue we use to visit everything (with `Seen` keeping us from revisiting). If I'm correct, `SmallSetVector` sounds like a sane thing here to me.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:520
+    BasicBlock *Exit = ExitBlocks[It];
+    // SimpleLoopUnswitch does not clone all exit blocks.
+    if (!VMap[Exit])
----------------
Please also note that uncloned exit blocks don't have predecessors/successors added


Repository:
  rL LLVM

https://reviews.llvm.org/D45299