[PATCH] D8688: Update MergedLoadStoreMotion to use MemorySSA
Gerolf Hoflehner via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 22 19:07:25 PDT 2016
Gerolf added a subscriber: Gerolf.
================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:297
@@ -231,3 +296,3 @@
MemoryLocation Loc0 = MemoryLocation::get(Load0);
MemoryLocation Loc1 = MemoryLocation::get(Load1);
----------------
Loc0,1 should be moved below the conditional. Then they are definitely needed.
================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:346
@@ -271,1 +345,3 @@
HoistedInst->insertBefore(HoistPt);
+ if (UseMemorySSA) {
+ // We also hoist operands of loads using this function, so check to see if
----------------
This is a MSSA update problem. How about
if (MSSA && MSSA->update(HoistCan, HoistedInst))
MSSA->removeMemoryAccess(ElseInst);
================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:542
@@ -448,1 +541,3 @@
assert(S1->getParent() == A1->getParent());
+ if (UseMemorySSA)
+ updateMemorySSAForSunkStores(BB, S0, S1, SNew);
----------------
You could remove the conditional here and simply return from updateMemorySSA when MSSA is null.
================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:617
@@ -521,1 +616,3 @@
+// \brief DFS Number instructions in blocks in dominator order, tracking where
+// the first and last hoist barriers are.
----------------
How about: Visit blocks in DFS order and number instructions. For each block track first hoist barrier and last sink barrier.
================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:745
@@ +744,3 @@
+ DominatorTree *DT = &AM.getResult<DominatorTreeAnalysis>(F);
+ if (UseMemorySSA)
+ MSSA = &AM.getResult<MemorySSAAnalysis>(F);
----------------
I think this is the only place where you need UseMemorySSA. All other instances can be handled by if (MSSA).
================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:1033
@@ +1032,3 @@
+
+void MergedLoadStoreMotion::updateMemorySSAForSunkStores(
+ BasicBlock *SinkLocation, StoreInst *S0, StoreInst *S1, StoreInst *SNew) {
----------------
Shouldn't updates be owned by MSSA rather than each client?
http://reviews.llvm.org/D8688
More information about the llvm-commits
mailing list