[PATCH] D8688: Update MergedLoadStoreMotion to use MemorySSA
Gerolf Hoflehner via llvm-commits
llvm-commits at lists.llvm.org
Tue Jul 5 20:42:57 PDT 2016
Gerolf added inline comments.
================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:809
@@ +808,3 @@
+ // accesses in the first.
+ for (auto AI = Succ1Accesses->begin(), AE = Succ1Accesses->end(); AI != AE;) {
+ const MemoryUse *MU = dyn_cast<MemoryUse>(&*AI);
----------------
The code from here could be a function. I prefer a function to fit on my screen.
================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:832
@@ +831,3 @@
+ bool SafeToLoadUnconditionally =
+ (LookupResult.first != LookupResult.second) &&
+ isSafeToLoadUnconditionally(Load1->getPointerOperand(),
----------------
Why do you need this? When the first condition is false the while loop does not execute. But then SafeToLoadUnconditionally is not needed anyway
================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:848
@@ +847,3 @@
+ if (BB0HoistBarrier != 0 &&
+ DFSNumberMap.lookup(Load0) > BB0HoistBarrier)
+ continue;
----------------
Within a block I thought the DFS numbers are
0
1
2
...
So I would expect loop up(load) < BBHoistBarrier. I'm probably wrong about the DFSs.
================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:850
@@ +849,3 @@
+ continue;
+ unsigned int BB1HoistBarrier =
+ FirstHoistBarrier.lookup(Load1->getParent());
----------------
Load1 is loop invariant and the continue condition can be computed in the header.
================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:862
@@ +861,3 @@
+ MergedLoads |= Res;
+ // Don't attempt to hoist above loads that had not been hoisted.
+ if (Res)
----------------
That comment does not look right. Load0 and load1 just got hoisted and LookupIter->first could still be Load0.
Also, not hoisting above loads that had not been hoisted is too restrictive. With better/cheaper DA this optimization can be more aggressive. Perhaps this could be a FIXME.
http://reviews.llvm.org/D8688
More information about the llvm-commits
mailing list