[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.

Some of the comments inline may be naive since I'm not very familiar with MemorySSA.



================
Comment at: lib/Transforms/Scalar/LICM.cpp:375
+  } else
+    delete MSSAUpdater;
 
----------------
Can this be a `std::unique_ptr`?


================
Comment at: lib/Transforms/Scalar/LICM.cpp:397
+         "Unexpected input to sinkRegion");
+  assert((CurAST != nullptr || (MSSAUpdater != nullptr && MSSA != nullptr)) &&
          "Unexpected input to sinkRegion");
----------------
Should this assert be checking `CurAST != nullptr ^ (MSSAUpdater != nullptr && MSSA != nullptr)`?


================
Comment at: lib/Transforms/Scalar/LICM.cpp:696
+        for (Value *Op : CI->arg_operands()) {
+          bool Invalidated = false;
+          if (CurAST)
----------------
Minor nit:  I usually prefer leaving variables like `Invalidated` uninitialized; that way if I forget to assign to it on one branch of the `if` I'll get a warning and/or an ASan failure.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:703
+                Op, MSSA, MSSA->getMemoryAccess(CI), CurLoop);
+          if (Op->getType()->isPointerTy() && Invalidated)
             return false;
----------------
We should probably early exit if `!Op->getType()->isPointerTy()` (and assert `Op->getType()->isPointerTy()` in `pointerInvalidatedByLoopWithMSSA` etc.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:711
       bool FoundMod = false;
-      for (AliasSet &AS : *CurAST) {
-        if (!AS.isForwardingAliasSet() && AS.isMod()) {
-          FoundMod = true;
-          break;
+      if (CurAST)
+        for (AliasSet &AS : *CurAST) {
----------------
Minor nit: even though unnecessary, I'd prefer using braces here since for readability.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:836
+  if (MSSA && (OldMemAcc = MSSA->getMemoryAccess(&I))) {
+    MemoryAccess *NewMemAcc = MSSAUpdater->createMemoryAccessInBB(
+        New, nullptr, New->getParent(), MemorySSA::Beginning);
----------------
I'm not sure that setting the defining access to nullptr here is correct -- if there is some subtle reason why it is, please add a comment.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:838
+        New, nullptr, New->getParent(), MemorySSA::Beginning);
+    MemoryDef *NewMemDef = dyn_cast_or_null<MemoryDef>(NewMemAcc);
+    if (NewMemDef)
----------------
Instead of two `dyn_cast_or_null`s how about:

```
if (NewMemAcc) {
  if (auto *MemDef = dyn_cast<MemoryDef>(NewMemAcc)) {
    ...
  } else {
    auto *MemUse = cast<MemoryUse>(NewMemAcc);
  }
}
```
?


================
Comment at: lib/Transforms/Scalar/LICM.cpp:840
+    if (NewMemDef)
+      MSSAUpdater->insertDef(NewMemDef, true);
+    else {
----------------
I'd prefer `/*RenameUses=*/true`.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:846
+        // This is only called to properly update the defining access
+        MSSA->getWalker()->getClobberingMemoryAccess(NewMemUse);
+      }
----------------
I'm not clear on how this works -- IIUC you're calling into `getClobberingMemoryAccess` with `NewMemUse`'s `DefiningAccess` set to `nullptr`, but:

 - Why don't we have to do this for `MemoryDef`s?
 - What if `MSSA->getWalker()` is a `DoNothingMemorySSAWalker`?
 - There are places where we should be crashing, like: `CachingWalker::getClobberingMemoryAccess` line 2078 that calls `getClobberingMemoryAccess(DefiningAccess, Q)` that calls `ClobberWalker::findWalker` which has `if (auto *MU = dyn_cast<MemoryUse>(Start))`.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1081-1082
+    // If moving, I just moved a load or store, so update MemorySSA.
+    MemoryUseOrDef *OldMemAcc =
+        dyn_cast_or_null<MemoryUseOrDef>(MSSA->getMemoryAccess(&I));
+    if (OldMemAcc) {
----------------
Can this be `cast_or_null`?


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1606
+    // results for small tests, and imprecise but fast for large ones.
+    // MemoryAccess *Source = MSSA->getWalker()->getClobberingMemoryAccess(MU);
+    MemoryAccess *Source = MU->getDefiningAccess();
----------------
Stray comment?


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1609
+    if (MSSA->isLiveOnEntryDef(Source))
+      return false;
+    if (!CurLoop->contains(Source->getBlock()))
----------------
Minor nit: probably cleaner to use `if (MSSA->isLiveOnEntryDef(Source) || !CurLoop->contains(Source->getBlock()))` here.


https://reviews.llvm.org/D40375