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

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added a comment.

(Thanks for the patch, by the way!)

So, I've looked at the API at a high level, and I think it's going in a good direction with some tweaks/naming changes. Naturally, whatever we end up standardizing on will need a fair amount of comments explaining what, exactly, it's trying to do/preconditions/etc. :)

FWIW, the naming suggestions are in part to help me be sure I'm not missing anything big WRT respected use-cases, ... So they're a bit shaky at times.



================
Comment at: include/llvm/Analysis/MemorySSA.h:780
+  MemoryUseOrDef *createNewAccess(Instruction *, bool IsKnownDef = false,
+                                  bool IsKnownUse = false);
   MemoryAccess *findDominatingDef(BasicBlock *, enum InsertionPlace);
----------------
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`)


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:378
 
+void MemorySSAUpdater::insertAccessesFromMap(BasicBlock *HeaderBlock,
+                                             ArrayRef<BasicBlock *> ExitBlocks,
----------------
`updateForClonedLoop`?

Given that we talk about entry blocks, exit blocks, etc. `insertAccessesFromMap` sounds pretty generic.


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


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


================
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,
----------------
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. :)


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:617
+                                  Instruction *Start,
+                                  MemorySSA::InsertionPlace Where) {
+
----------------
I don't see a case in the given usage examples where this needs to be anything but `End`, and I suspect this gets a bit more complicated if `Where != End`. Until we need that functionality, can we please just assume it's always `End` for now? (D45300 uses `Beginning`, but AFAICT, it's a clean BB anyway. So, `End` should be equivalent?)


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:651
+
+  // Update MemoryPhis in all successors to replace incoming Old with New.
+  SmallPtrSet<BasicBlock *, 8> Seen;
----------------
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`)


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:686
 
+void MemorySSAUpdater::updatePhisForSplitBlock(BasicBlock *Old, BasicBlock *New,
+                                               ArrayRef<BasicBlock *> Preds) {
----------------
`movePhiToNewImmediatePredecessor` maybe?

It's a bit of a mouthful, yeah...


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:703
+  // Alternative?
+  // removeFromLists(Phi, ShouldDelete=false)
+  // insertIntoListsForBlock(Phi, New, MemorySSA::Beginning);
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D45299