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

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added inline comments.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:397
+         "Unexpected input to sinkRegion");
+  assert((CurAST != nullptr || (MSSAUpdater != nullptr && MSSA != nullptr)) &&
          "Unexpected input to sinkRegion");
----------------
sanjoy wrote:
> Should this assert be checking `CurAST != nullptr ^ (MSSAUpdater != nullptr && MSSA != nullptr)`?
At some point I was testing with both enabled, but considering the added compile time when doing that, yes :).


================
Comment at: lib/Transforms/Scalar/LICM.cpp:846
+        // This is only called to properly update the defining access
+        MSSA->getWalker()->getClobberingMemoryAccess(NewMemUse);
+      }
----------------
sanjoy wrote:
> 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))`.
AFAIU:
For Defs the DefiningAccess is properly updated.
For Uses, there is a DefiningAccess set, but that one is not optimized. When adding an access like in this case, calling getClobberingMemoryAccess will optimize it.
If the walker is DoNothingMemorySSAWalker, it won't optimize?

Pulling in george.burgess.iv to clarify, it's possible I'm missing something here.



https://reviews.llvm.org/D40375