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

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added a comment.
This revision now requires changes to proceed.

I didn't carefully review the MSSA specific stuff since you're the domain expert here.  I do have some general comments.



================
Comment at: lib/Transforms/Scalar/LICM.cpp:110
 
+// Experimental option to allow imprecision in LICM (use MemorySSA cap) in
+// pathological cases, in exchange for faster compile. This is to be removed
----------------
Not sure if it is beneficial to split out a separate `[Verbose]` section -- maybe we can combine both into a large single comment?


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


================
Comment at: lib/Transforms/Scalar/LICM.cpp:655
       HoistDestinationMap[Orig] = New;
       DT->addNewBlock(New, HoistTarget);
       if (CurLoop->getParentLoop())
----------------
Just to be clear, we don't have to tell `MSSAU` about these new blocks (and the edges between these new blocks)?


================
Comment at: lib/Transforms/Scalar/LICM.cpp:964
 }
 /// Return true if all of the alias sets within this AST are known not to
 /// contain a Mod.
----------------
Update comment?


================
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;
----------------
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;
```



================
Comment at: lib/Transforms/Scalar/LICM.cpp:1122
+    } else { // MSSAU
+      if (isOnlyMemoryAccess(FI, CurLoop, MSSAU))
+        return true;
----------------
Can be `return isOnlyMemoryAccess(FI, CurLoop, MSSAU)`.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D40375