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

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added inline comments.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1
-//===-- LICM.cpp - Loop Invariant Code Motion Pass ------------------------===//
+
 //
----------------
Stray edit?


================
Comment at: lib/Transforms/Scalar/LICM.cpp:99
+// AliasSetTracker.
+// When flag is disabled (default), LICM calls MemorySSAWalker's getClobberingMemoryAccess, which gets perfect accuracy.
+// When flag is enabled, LICM will call into MemorySSA's
----------------
Line too long?


================
Comment at: lib/Transforms/Scalar/LICM.cpp:420
+         "Unexpected input to sinkRegion");
+  assert(((CurAST != nullptr) ^ (MSSAUpdater != nullptr && MSSA != nullptr)) &&
          "Unexpected input to sinkRegion");
----------------
Probably better to have two asserts here:

 - `assert((MSSAUpdater == nullptr) == (MSSA == nullptr))`
 - `assert((CurAST != nullptr) ^ (MSSA != nullptr))`


================
Comment at: lib/Transforms/Scalar/LICM.cpp:905
+        auto *MemUse = cast<MemoryUse>(NewMemAcc);
+        MSSAUpdater->insertUse(MemUse);
+        // This is only called to properly update the defining access.
----------------
Note: I did not revisit this bit since it looks like @george.burgess.iv settled it.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1159
+      if (EnableLicmCap)
+        MSSA->getWalker()->getClobberingMemoryAccess(OldMemAcc);
+    }
----------------
I not convinced this is a good idea -- it looks like we're coding to the implementation of `getClobberingMemoryAccess` here instead of coding to its interface.  But if @george.burgess.iv is fine with it, I am too.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1159
+      if (EnableLicmCap)
+        MSSA->getWalker()->getClobberingMemoryAccess(OldMemAcc);
+    }
----------------
sanjoy wrote:
> I not convinced this is a good idea -- it looks like we're coding to the implementation of `getClobberingMemoryAccess` here instead of coding to its interface.  But if @george.burgess.iv is fine with it, I am too.
It looks like `MemorySSA::CachingWalker::getClobberingMemoryAccess` has an early exit if `OldMemAcc` was optimized.  Are we okay not updating the defining access in that case?


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1672
+  // MUD is from a LoopInstr or CallInstr, so both Uses.
+  if (MemoryUse *MU = dyn_cast_or_null<MemoryUse>(MUD)) {
+    MemoryAccess *Source;
----------------
If `MUD` is always guaranteed to be a use then this should be `cast_or_null`.  Otherwise please update the comment.


Repository:
  rL LLVM

https://reviews.llvm.org/D40375