[PATCH] D40375: Use MemorySSA in LICM to do sinking and hoisting.

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added a comment.

I also cleaned up the usage of EnableLicmCap. It is only used in 2 places now, with no forced calls to getClobbering (the issue in MemorySSA requiring this is now resolved).
FWIW, I'm planning to replace this with an integer cap in a future patch (i.e. allow a limited number of calls to getClobbering, vs all or none like we have now).



================
Comment at: lib/Transforms/Scalar/LICM.cpp:124
+// perfect results for small tests, and imprecise but fast for large ones.
+static cl::opt<bool> EnableLicmCap(
+    "enable-licm-cap", cl::init(false), cl::Hidden,
----------------
sanjoy wrote:
> Just to be clear, if this is true then `opt -licm -some_pass_using_mssa` is not the same as `opt -licm | opt -some_pass_using_mssa`?  If yes, that needs  to be explicitly (and loudly) mentioned.
Hmm, let me think this out loud.

If the flag is true, there may be more opportunities for sink/hoist found at the cost of compile time, so the end result (module) may be different. But the **users** of the resulting MemorySSA should not be affected by the flag being true or false.

There may be more things already optimized in MemorySSA with the flag set to false.
And more things to be optimized on demand with the flag set to true.
So printing MemorySSA after LICM may have differences.
But the pass running after LICM should get the same results to MemorySSA clobbering queries.

Does this make sense?


================
Comment at: lib/Transforms/Scalar/LICM.cpp:655
       HoistDestinationMap[Orig] = New;
       DT->addNewBlock(New, HoistTarget);
       if (CurLoop->getParentLoop())
----------------
sanjoy wrote:
> Just to be clear, we don't have to tell `MSSAU` about these new blocks (and the edges between these new blocks)?
`MSSAU` doesn't keep track of blocks with no memory accesses.  This code if just duplicating the control flow from inside the loop to outside the loop. `MSSAU` only starts caring when instructions that touch memory are moved in there.


It also matters that the correct block predecessors are included in MemoryPhis, so when control flow changes to modify that, we need to update `MSSA`. The 3 lines below address this.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:995
+        // if one of the two accesses is not a phi, ret false.
+        if (!dyn_cast<MemoryPhi>(&*Acc->begin()))
+          return false;
----------------
sanjoy wrote:
> So if there is a memory phi then it has to be `Acc->begin()`?
> 
> Btw, it might be cleaner to express this as a loop:
> 
> ```
> int nonphi = 0;
> for (auto *X : *Acc) {
>   if (is_phi) { continue; }
>   if (X->getInst() != I || nonphi++ == 1) { return false; }
> }
> return true;
> ```
> 
Yes, a block may have a single `MemoryPhi`, and it's always the first one in the list of Defs.

SG! Updated with loop.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D40375/new/

https://reviews.llvm.org/D40375