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

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added inline comments.


================
Comment at: include/llvm/Analysis/MemorySSA.h:780
+  MemoryUseOrDef *createNewAccess(Instruction *, bool IsKnownDef = false,
+                                  bool IsKnownUse = false);
   MemoryAccess *findDominatingDef(BasicBlock *, enum InsertionPlace);
----------------
george.burgess.iv wrote:
> asbirlea wrote:
> > dberlin wrote:
> > > I don't understand the desire here, since it already should correctly identify whether it is a def or use on it's own.
> > > What are you trying to do?
> > I'm trying to avoid the additional getModRefInfo call. 
> > 
> > For cloned blocks we have a 1:1 mapping of all instructions. E.g., we know we've just cloned I, with MUD access that's a Use with some defining access. I'm trying to create a MUD for the clone instruction with all that info. The use case is in "CloneUsesAndDefs" below.
> > 
> > Perhaps it's not worth the effort, just trying to avoid AA calls where possible.
> Would it be clearer to pass `MemoryUseOrDef *Template` and have this function interrogate it, instead of `bool Use` and `bool Def` (or split off a `cloneMemoryAccess` function to go with the whole cloning theme we're doing)? It's a bit more explicit about what you're trying to do (IMO), and it looks like it would simplify calling code a bit.
> 
> (Same with `createDefinedAccess`)
Updated to `MemoryUseOrDef *Template = nullptr`.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:508
+
+void MemorySSAUpdater::insertPhisIfNeeded(BasicBlock *BB, BasicBlock *P1,
+                                          DominatorTree *DT) {
----------------
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? :-/


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:542
+
+  SmallPtrSet<BasicBlock *, 8> Seen;
+  SmallVector<BasicBlock *, 16> WorklistBB;
----------------
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.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:615
+// All accesses in To used to be in From. Update access lists.
+void MemorySSAUpdater::moveInBulk(BasicBlock *From, BasicBlock *To,
+                                  Instruction *Start,
----------------
george.burgess.iv wrote:
> So... "I just moved everything from `Start` to the end of `To` from `From` to `To` (I'm so glad we have backticks to disambiguate code from words). Please update MSSA to note that." ?
> 
> I'm assuming there are serious constraints on when this can be used. Moreover, I'm assuming that any use is invalid unless either:
> 
> - `From` was merged into `To` and is about to be deleted entirely, and `From`/`To` logically formed one basic block before the merge (e.g. `From`'s only predecessor was `To`, and `To`'s only successor was `From`)
> - ...Or `To` is the result of chopping `From` in half starting at `Start`, so they still both logically form one basic block (with the pred/succ constraints mentioned above)
> 
> So, from a wording standpoint, I sort of think we should spell out both of those use-cases as separate methods (`mergeSplicedBlocks` and `spliceBlocks`?). If they both end up as one-line wrappers around this private `moveInBulk`, I'm happy with that. I'd just rather not advertise "hey, this is a general bulk move operation. Come to it with all your bulk memory moving needs." Plus, if we're specific in what we allow, we can throw in more asserts to catch mistakes. :)
You're right. Created `moveAllAfterSpliceBlocks` and `moveAllAfterMergeBlocks`.
Both call private member `moveAllAccesses`.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:651
+
+  // Update MemoryPhis in all successors to replace incoming Old with New.
+  SmallPtrSet<BasicBlock *, 8> Seen;
----------------
george.burgess.iv wrote:
> Why can't this just be "for each user of the last def in `To` that happens to be a Phi and has `From` as the old block, update its basic block"?
> 
> (We'd also need to special-case when `Where != End` here, probably, since the last Def in `From` might not be the last Def in `To`)
This can be called after optimizeUses and some defs may be optimized too. Doesn't this mean that any moved def in `To` can have users in a Phi, not just the last one?

It should work to go over all Defs in To, and all their users and if those are Phis with `From` incoming update to `To`. I think that may be more expensive, if there are a lot of Defs and few successors (transitive too), but it can go both ways.



================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:703
+  // Alternative?
+  // removeFromLists(Phi, ShouldDelete=false)
+  // insertIntoListsForBlock(Phi, New, MemorySSA::Beginning);
----------------
george.burgess.iv wrote:
> Yeah. This seems like the better approach to me, since this phi we're creating is identical (from the user's perspective, at least) to the previous one. Faster + (I assume) easier to think about + less code = yay.
Done, but I needed to modify MemorySSA's API to include `MemoryPhis`: generalized to `moveTo(MemoryAccess *)`.

AFAICT this should be fine, since that call can only used by the updater, and this is the only case where a MemoryPhi is moved.
The alternative is to add another MemorySSA API: `moveTo(MemoryPhi *, BasicBlock *)` and keep the old one with `MemoryUseOrDef`. The advantage being the removal of the insertion point. I don't have a strong opinion here.

PTAL.


Repository:
  rL LLVM

https://reviews.llvm.org/D45299