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

Alina Sbirlea via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Dec 21 16:33:26 PST 2017

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))`.
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.


More information about the llvm-commits mailing list