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

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea marked 32 inline comments as done.
asbirlea added a comment.

I'm planning to add a few more unittests, but sending the updates now for additional comments.



================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:604
+    // not occured.
+    DominatorTree NewDT(DT, RevDeleteUpdates);
+    GraphDiff<BasicBlock *> GD(RevDeleteUpdates);
----------------
george.burgess.iv wrote:
> nit: Is it possible/easy to update DT in-place and undo the updates? The tool I'm using to grok this says that this line always lands in CalculateWithUpdates, which appears to always call CalculateFromScratch. It sometimes gets things wrong with templated code, though, so I could be assuming the wrong thing here.
At the moment  we are creating a new DT. I should be able to optimize this after cleaning up DT to use GraphDiff internally. For now, added a comment.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:703
+    auto &PrevBlockSet = PrevPredMap[BB];
+    for (auto &Pair : children<GraphDiffInvBBPair>({GD, BB})) {
+      BasicBlock *Pi = Pair.second;
----------------
george.burgess.iv wrote:
> To be sure I'm understanding (I'm not overly familiar with the DT APIs), this is saying "for all predecessors of BB, but also including additions in GD, { /* loop body */ }", yeah?
Yes. The additions in GD in this case are edges that were removed and that we're reinserting for the scope of this update. 


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:760
+
+      // FIXME: May still need to check defs that no longer dominate their
+      // uses. In particular defs optimized through NewPhi?
----------------
george.burgess.iv wrote:
> Consider:
> 
> ```
>    entry
>      |
>      a
>     / \
>    |   c
>     \ /
>      d
> 
> a:
>   ; 1 = MemoryDef(liveOnEntry)
> c:
>   ; 2 = MemoryDef(1)
> d:
>   ; 3 = MemoryPhi({c, 2}, {a, 1})
>   ; 4 = MemoryDef(3); optimized to 1
> ```
> 
> And we add an edge from `entry` -> `d`.  I don't claim to know when/if this can reasonably happen, but ...
Fixed. Even if a Phi exists, continue to the section below that updates defs in BlocksWithDefsToReplace. Added test case as well.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:807
+           "Block with last definition must dominate idom");
+    GetNoLongerDomBlocks(PrevIDom, NewIDom, BlocksWithDefsToReplace);
+
----------------
george.burgess.iv wrote:
> Is it possible for BlocksWithDefsToReplace to get duplicates? If so, might it be a good idea to replace that with a SmallPtrSet?
We cannot get duplicates. We're walking up the DT, no cycles.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:816
+  ForwardIDFCalculator IDFs(DT, GD);
+  SmallPtrSet<BasicBlock *, 16> DefiningBlocks(BlocksToProcess.begin(),
+                                               BlocksToProcess.end());
----------------
george.burgess.iv wrote:
> (I'm assuming that there's some sort of guarantee that IDF calculations only pass these BBs into the DT. If it tries to access preds/succs of a BB, we might get badness, since the DT here may be pretending that we have CFG edges that we don't? Or did I miss us temporarily re-adding edges that were deleted between BBs?)
The GD passes as argument to IDF takes care of that.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:832
+  // longer dominate, replace those with the closest dominating def.
+  for (auto *BlockWithDefsToReplace : BlocksWithDefsToReplace) {
+    if (auto DefsList = MSSA->getWritableBlockDefs(BlockWithDefsToReplace)) {
----------------
george.burgess.iv wrote:
> Are we sure that we only need to consider BlocksWithDefsToReplace here?
> 
> I could be totally misunderstanding, but imagine the following scenario (I'm not even going to try to invent an excuse for when this could happen. :) Just wanted to know if it was considered):
> 
> ```
>    entry
>    /  |
>   a   |
>  / \  |
>  b c  f
>  \ /  |
>   d   |
>    \ /
>     e
> 
> f:
>   ; 1 = MemoryDef(liveOnEntry)
> e:
>   ; 2 = MemoryPhi({d, liveOnEntry}, {f, 1})
> ```
> 
> Now imagine that we add an edge `f` -> `c`. AFAICT, that would cause us to add a Phi to `c` (way above) and a Phi to `d` (idf loop), but I'm failing to see where we'd update the Phi in `e` to point to the new Phi in `d`. (Since the only no-longer-dominating BB is `a`)
> 
> Similarly, if `d` had a MemoryDef in it before we added this edge, would its DefiningAccess get pointed appropriately at this new Phi in `d`?
This should be handled in the IDF loop above, fixed there.
Added testcase for this scenario.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:839
+                            E = DefToReplaceUses.use_end();
+        for (; UI != E;) {
+          Use &U = *UI;
----------------
george.burgess.iv wrote:
> nit: Can we just do a range-for loop? Looks like we're only calling `set` on the use, which shouldn't cause any invalidation badness.
I may be missing something, but if I do: 
`for (Use &U : DefToReplaceUses.uses())`, I get a crash.
I guess the `set()` does invalidate the iterator? See below, the `++UI` needs to be before `U.set()`.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:852
+            if (!DT.dominates(DominatingBlock, DominatedBlock)) {
+              for (auto *BB :
+                   llvm::concat<BasicBlock *>(BlocksToProcess, IDFBlocks))
----------------
george.burgess.iv wrote:
> The `O(Blocks.size() * AverageBlockMemoryAccessesCount * AverageMemoryAccessUserCount * (Blocks.size() + IDF.size()))` (assuming `dominates` is constant time) behavior here is somewhat concerning to me, though idk if that's an unfounded concern.
> 
> If the user is a Def, it's probably best to just deoptimize it; we'll just start from the Def's defining access in the walker anyway. (...Or, well, we would if we noticed that the optimization is now invalid. For reasons unclear to me looking into history + comments, `MemoryDef::isOptimized` checks the Def's *defining* access, rather than its optimized access. Which is almost definitely incorrect, at least with the aliasing bits we now cache. Working on that now.)
> 
> If it's not the Def's optimized access that we're dealing with, then the only new Access that can work is `MSSA->getMemoryAccess(U->getBlock());` (since the only way that can happen is if that Def is the first access in a BB, and we've just inserted that block into said BB).
> 
> If it's a Use, we'll try to reoptimize starting from its DefiningAccess, which will be whatever this Use points to. It's unclear what the right trade-off is there, since the more aggressive we are here, the less walking we *may* have to do in the future. Depending on the amount of luck involved in this loop's iteration order (e.g. if we remove the `break;`, is it possible for `U` to point to something different when the loop terminates? Is that value going to be better, worse, or are we uncertain? The feeling I get is "yes; uncertain," but I haven't fully reasoned through it,) it may be best to just say "point it at the phi of the Use's BB. If that doesn't exist, walk IDoms until we find the last Def."
> 
> (Edit: ...Did I never make MemoryDefs stop using WeakVHes to track what they're optimized to? OK. I'll work on that in parallel, too, since I imagine that's pretty necessary for this to work.)
This should be fixed. Added testcase as well.


Repository:
  rL LLVM

https://reviews.llvm.org/D45299