<div dir="ltr">(and note, you didn't explain, anywhere, why any of these changes are necessary for the new PM, they look, at a glance, like completely orthogonal refactorings)</div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jun 16, 2016 at 9:07 AM, Daniel Berlin <span dir="ltr"><<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">I don't remember seeing this for review, and you've basically taken a very well organized class and completely destroyed its organizational structure by creating a mix of class member functions and random non-class static functions.<div><br><div>I don't see this as an improvement, even for the new PM.</div><div>I also don't think we should try to take every single pass that is a class in LLVM and necessarily turn it into a bunch of static functions that pass around what was previously class state. </div><div><br></div><div>I'm going to ask you to revert and actually post a patch for review so this can be actually discussed.</div><div><br></div><div><br></div><div><br></div></div></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jun 13, 2016 at 3:27 PM, Davide Italiano via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: davide<br>
Date: Mon Jun 13 17:27:30 2016<br>
New Revision: 272595<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=272595&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=272595&view=rev</a><br>
Log:<br>
[PM/MergeLoadStoreMotion] Convert the logic to static functions.<br>
<br>
Pass AliasAnalyis and MemoryDepResult around. This is in preparation<br>
for porting this pass to the new PM.<br>
<br>
Modified:<br>
llvm/trunk/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp<br>
<br>
Modified: llvm/trunk/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp?rev=272595&r1=272594&r2=272595&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp?rev=272595&r1=272594&r2=272595&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp Mon Jun 13 17:27:30 2016<br>
@@ -103,8 +103,7 @@ class MergedLoadStoreMotion : public Fun<br>
<br>
public:<br>
static char ID; // Pass identification, replacement for typeid<br>
- MergedLoadStoreMotion()<br>
- : FunctionPass(ID), MD(nullptr), MagicCompileTimeControl(250) {<br>
+ MergedLoadStoreMotion() : FunctionPass(ID), MD(nullptr) {<br>
initializeMergedLoadStoreMotionPass(*PassRegistry::getPassRegistry());<br>
}<br>
<br>
@@ -118,43 +117,17 @@ private:<br>
AU.addPreserved<GlobalsAAWrapperPass>();<br>
AU.addPreserved<MemoryDependenceWrapperPass>();<br>
}<br>
-<br>
- // Helper routines<br>
-<br>
- ///<br>
- /// \brief Remove instruction from parent and update memory dependence<br>
- /// analysis.<br>
- ///<br>
- void removeInstruction(Instruction *Inst);<br>
- BasicBlock *getDiamondTail(BasicBlock *BB);<br>
- bool isDiamondHead(BasicBlock *BB);<br>
- // Routines for hoisting loads<br>
- bool isLoadHoistBarrierInRange(const Instruction &Start,<br>
- const Instruction &End, LoadInst *LI,<br>
- bool SafeToLoadUnconditionally);<br>
- LoadInst *canHoistFromBlock(BasicBlock *BB, LoadInst *LI);<br>
- void hoistInstruction(BasicBlock *BB, Instruction *HoistCand,<br>
- Instruction *ElseInst);<br>
- bool isSafeToHoist(Instruction *I) const;<br>
- bool hoistLoad(BasicBlock *BB, LoadInst *HoistCand, LoadInst *ElseInst);<br>
- bool mergeLoads(BasicBlock *BB);<br>
- // Routines for sinking stores<br>
- StoreInst *canSinkFromBlock(BasicBlock *BB, StoreInst *SI);<br>
- PHINode *getPHIOperand(BasicBlock *BB, StoreInst *S0, StoreInst *S1);<br>
- bool isStoreSinkBarrierInRange(const Instruction &Start,<br>
- const Instruction &End, MemoryLocation Loc);<br>
- bool sinkStore(BasicBlock *BB, StoreInst *SinkCand, StoreInst *ElseInst);<br>
- bool mergeStores(BasicBlock *BB);<br>
- // The mergeLoad/Store algorithms could have Size0 * Size1 complexity,<br>
- // where Size0 and Size1 are the #instructions on the two sides of<br>
- // the diamond. The constant chosen here is arbitrary. Compiler Time<br>
- // Control is enforced by the check Size0 * Size1 < MagicCompileTimeControl.<br>
- const int MagicCompileTimeControl;<br>
};<br>
<br>
char MergedLoadStoreMotion::ID = 0;<br>
} // anonymous namespace<br>
<br>
+// The mergeLoad/Store algorithms could have Size0 * Size1 complexity,<br>
+// where Size0 and Size1 are the #instructions on the two sides of<br>
+// the diamond. The constant chosen here is arbitrary. Compiler Time<br>
+// Control is enforced by the check Size0 * Size1 < MagicCompileTimeControl.<br>
+const int MagicCompileTimeControl = 250;<br>
+<br>
///<br>
/// \brief createMergedLoadStoreMotionPass - The public interface to this file.<br>
///<br>
@@ -173,7 +146,7 @@ INITIALIZE_PASS_END(MergedLoadStoreMotio<br>
///<br>
/// \brief Remove instruction from parent and update memory dependence analysis.<br>
///<br>
-void MergedLoadStoreMotion::removeInstruction(Instruction *Inst) {<br>
+static void removeInstruction(Instruction *Inst, MemoryDependenceResults *MD) {<br>
// Notify the memory dependence analysis.<br>
if (MD) {<br>
MD->removeInstruction(Inst);<br>
@@ -187,17 +160,9 @@ void MergedLoadStoreMotion::removeInstru<br>
}<br>
<br>
///<br>
-/// \brief Return tail block of a diamond.<br>
-///<br>
-BasicBlock *MergedLoadStoreMotion::getDiamondTail(BasicBlock *BB) {<br>
- assert(isDiamondHead(BB) && "Basic block is not head of a diamond");<br>
- return BB->getTerminator()->getSuccessor(0)->getSingleSuccessor();<br>
-}<br>
-<br>
-///<br>
/// \brief True when BB is the head of a diamond (hammock)<br>
///<br>
-bool MergedLoadStoreMotion::isDiamondHead(BasicBlock *BB) {<br>
+static bool isDiamondHead(BasicBlock *BB) {<br>
if (!BB)<br>
return false;<br>
auto *BI = dyn_cast<BranchInst>(BB->getTerminator());<br>
@@ -221,15 +186,24 @@ bool MergedLoadStoreMotion::isDiamondHea<br>
}<br>
<br>
///<br>
+/// \brief Return tail block of a diamond.<br>
+///<br>
+static BasicBlock *getDiamondTail(BasicBlock *BB) {<br>
+ assert(isDiamondHead(BB) && "Basic block is not head of a diamond");<br>
+ return BB->getTerminator()->getSuccessor(0)->getSingleSuccessor();<br>
+}<br>
+<br>
+///<br>
/// \brief True when instruction is a hoist barrier for a load<br>
///<br>
/// Whenever an instruction could possibly modify the value<br>
/// being loaded or protect against the load from happening<br>
/// it is considered a hoist barrier.<br>
///<br>
-bool MergedLoadStoreMotion::isLoadHoistBarrierInRange(<br>
- const Instruction &Start, const Instruction &End, LoadInst *LI,<br>
- bool SafeToLoadUnconditionally) {<br>
+static bool isLoadHoistBarrierInRange(const Instruction &Start,<br>
+ const Instruction &End, LoadInst *LI,<br>
+ bool SafeToLoadUnconditionally,<br>
+ AliasAnalysis *AA) {<br>
if (!SafeToLoadUnconditionally)<br>
for (const Instruction &Inst :<br>
make_range(Start.getIterator(), End.getIterator()))<br>
@@ -246,8 +220,8 @@ bool MergedLoadStoreMotion::isLoadHoistB<br>
/// and it can be hoisted from \p BB, return that load.<br>
/// Otherwise return Null.<br>
///<br>
-LoadInst *MergedLoadStoreMotion::canHoistFromBlock(BasicBlock *BB1,<br>
- LoadInst *Load0) {<br>
+static LoadInst *canHoistFromBlock(BasicBlock *BB1, LoadInst *Load0,<br>
+ AliasAnalysis *AA) {<br>
BasicBlock *BB0 = Load0->getParent();<br>
BasicBlock *Head = BB0->getSinglePredecessor();<br>
bool SafeToLoadUnconditionally = isSafeToLoadUnconditionally(<br>
@@ -267,9 +241,9 @@ LoadInst *MergedLoadStoreMotion::canHois<br>
MemoryLocation Loc1 = MemoryLocation::get(Load1);<br>
if (AA->isMustAlias(Loc0, Loc1) && Load0->isSameOperationAs(Load1) &&<br>
!isLoadHoistBarrierInRange(BB1->front(), *Load1, Load1,<br>
- SafeToLoadUnconditionally) &&<br>
+ SafeToLoadUnconditionally, AA) &&<br>
!isLoadHoistBarrierInRange(BB0->front(), *Load0, Load0,<br>
- SafeToLoadUnconditionally)) {<br>
+ SafeToLoadUnconditionally, AA)) {<br>
return Load1;<br>
}<br>
}<br>
@@ -282,9 +256,9 @@ LoadInst *MergedLoadStoreMotion::canHois<br>
///<br>
/// BB is the head of a diamond<br>
///<br>
-void MergedLoadStoreMotion::hoistInstruction(BasicBlock *BB,<br>
- Instruction *HoistCand,<br>
- Instruction *ElseInst) {<br>
+static void hoistInstruction(BasicBlock *BB, Instruction *HoistCand,<br>
+ Instruction *ElseInst,<br>
+ MemoryDependenceResults *MD) {<br>
DEBUG(dbgs() << " Hoist Instruction into BB \n"; BB->dump();<br>
dbgs() << "Instruction Left\n"; HoistCand->dump(); dbgs() << "\n";<br>
dbgs() << "Instruction Right\n"; ElseInst->dump(); dbgs() << "\n");<br>
@@ -305,16 +279,16 @@ void MergedLoadStoreMotion::hoistInstruc<br>
HoistedInst->insertBefore(HoistPt);<br>
<br>
HoistCand->replaceAllUsesWith(HoistedInst);<br>
- removeInstruction(HoistCand);<br>
+ removeInstruction(HoistCand, MD);<br>
// Replace the else block instruction.<br>
ElseInst->replaceAllUsesWith(HoistedInst);<br>
- removeInstruction(ElseInst);<br>
+ removeInstruction(ElseInst, MD);<br>
}<br>
<br>
///<br>
/// \brief Return true if no operand of \p I is defined in I's parent block<br>
///<br>
-bool MergedLoadStoreMotion::isSafeToHoist(Instruction *I) const {<br>
+static bool isSafeToHoist(Instruction *I) {<br>
BasicBlock *Parent = I->getParent();<br>
for (Use &U : I->operands())<br>
if (auto *Instr = dyn_cast<Instruction>(&U))<br>
@@ -326,8 +300,8 @@ bool MergedLoadStoreMotion::isSafeToHois<br>
///<br>
/// \brief Merge two equivalent loads and GEPs and hoist into diamond head<br>
///<br>
-bool MergedLoadStoreMotion::hoistLoad(BasicBlock *BB, LoadInst *L0,<br>
- LoadInst *L1) {<br>
+static bool hoistLoad(BasicBlock *BB, LoadInst *L0, LoadInst *L1,<br>
+ MemoryDependenceResults *MD) {<br>
// Only one definition?<br>
auto *A0 = dyn_cast<Instruction>(L0->getPointerOperand());<br>
auto *A1 = dyn_cast<Instruction>(L1->getPointerOperand());<br>
@@ -338,8 +312,8 @@ bool MergedLoadStoreMotion::hoistLoad(Ba<br>
DEBUG(dbgs() << "Hoist Instruction into BB \n"; BB->dump();<br>
dbgs() << "Instruction Left\n"; L0->dump(); dbgs() << "\n";<br>
dbgs() << "Instruction Right\n"; L1->dump(); dbgs() << "\n");<br>
- hoistInstruction(BB, A0, A1);<br>
- hoistInstruction(BB, L0, L1);<br>
+ hoistInstruction(BB, A0, A1, MD);<br>
+ hoistInstruction(BB, L0, L1, MD);<br>
return true;<br>
}<br>
return false;<br>
@@ -351,7 +325,8 @@ bool MergedLoadStoreMotion::hoistLoad(Ba<br>
/// Starting from a diamond head block, iterate over the instructions in one<br>
/// successor block and try to match a load in the second successor.<br>
///<br>
-bool MergedLoadStoreMotion::mergeLoads(BasicBlock *BB) {<br>
+static bool mergeLoads(BasicBlock *BB, AliasAnalysis *AA,<br>
+ MemoryDependenceResults *MD) {<br>
bool MergedLoads = false;<br>
assert(isDiamondHead(BB));<br>
BranchInst *BI = cast<BranchInst>(BB->getTerminator());<br>
@@ -373,8 +348,8 @@ bool MergedLoadStoreMotion::mergeLoads(B<br>
++NLoads;<br>
if (NLoads * Size1 >= MagicCompileTimeControl)<br>
break;<br>
- if (LoadInst *L1 = canHoistFromBlock(Succ1, L0)) {<br>
- bool Res = hoistLoad(BB, L0, L1);<br>
+ if (LoadInst *L1 = canHoistFromBlock(Succ1, L0, AA)) {<br>
+ bool Res = hoistLoad(BB, L0, L1, MD);<br>
MergedLoads |= Res;<br>
// Don't attempt to hoist above loads that had not been hoisted.<br>
if (!Res)<br>
@@ -392,9 +367,9 @@ bool MergedLoadStoreMotion::mergeLoads(B<br>
/// value being stored or protect against the store from<br>
/// happening it is considered a sink barrier.<br>
///<br>
-bool MergedLoadStoreMotion::isStoreSinkBarrierInRange(const Instruction &Start,<br>
- const Instruction &End,<br>
- MemoryLocation Loc) {<br>
+static bool isStoreSinkBarrierInRange(const Instruction &Start,<br>
+ const Instruction &End,<br>
+ MemoryLocation Loc, AliasAnalysis *AA) {<br>
for (const Instruction &Inst :<br>
make_range(Start.getIterator(), End.getIterator()))<br>
if (Inst.mayThrow())<br>
@@ -407,8 +382,8 @@ bool MergedLoadStoreMotion::isStoreSinkB<br>
///<br>
/// \return The store in \p when it is safe to sink. Otherwise return Null.<br>
///<br>
-StoreInst *MergedLoadStoreMotion::canSinkFromBlock(BasicBlock *BB1,<br>
- StoreInst *Store0) {<br>
+static StoreInst *canSinkFromBlock(BasicBlock *BB1, StoreInst *Store0,<br>
+ AliasAnalysis *AA) {<br>
DEBUG(dbgs() << "can Sink? : "; Store0->dump(); dbgs() << "\n");<br>
BasicBlock *BB0 = Store0->getParent();<br>
for (BasicBlock::reverse_iterator RBI = BB1->rbegin(), RBE = BB1->rend();<br>
@@ -422,8 +397,10 @@ StoreInst *MergedLoadStoreMotion::canSin<br>
MemoryLocation Loc0 = MemoryLocation::get(Store0);<br>
MemoryLocation Loc1 = MemoryLocation::get(Store1);<br>
if (AA->isMustAlias(Loc0, Loc1) && Store0->isSameOperationAs(Store1) &&<br>
- !isStoreSinkBarrierInRange(*Store1->getNextNode(), BB1->back(), Loc1) &&<br>
- !isStoreSinkBarrierInRange(*Store0->getNextNode(), BB0->back(), Loc0)) {<br>
+ !isStoreSinkBarrierInRange(*Store1->getNextNode(), BB1->back(), Loc1,<br>
+ AA) &&<br>
+ !isStoreSinkBarrierInRange(*Store0->getNextNode(), BB0->back(), Loc0,<br>
+ AA)) {<br>
return Store1;<br>
}<br>
}<br>
@@ -433,8 +410,8 @@ StoreInst *MergedLoadStoreMotion::canSin<br>
///<br>
/// \brief Create a PHI node in BB for the operands of S0 and S1<br>
///<br>
-PHINode *MergedLoadStoreMotion::getPHIOperand(BasicBlock *BB, StoreInst *S0,<br>
- StoreInst *S1) {<br>
+static PHINode *getPHIOperand(BasicBlock *BB, StoreInst *S0, StoreInst *S1,<br>
+ MemoryDependenceResults *MD) {<br>
// Create a phi if the values mismatch.<br>
Value *Opd1 = S0->getValueOperand();<br>
Value *Opd2 = S1->getValueOperand();<br>
@@ -455,8 +432,8 @@ PHINode *MergedLoadStoreMotion::getPHIOp<br>
///<br>
/// Also sinks GEP instruction computing the store address<br>
///<br>
-bool MergedLoadStoreMotion::sinkStore(BasicBlock *BB, StoreInst *S0,<br>
- StoreInst *S1) {<br>
+static bool sinkStore(BasicBlock *BB, StoreInst *S0, StoreInst *S1,<br>
+ MemoryDependenceResults *MD) {<br>
// Only one definition?<br>
auto *A0 = dyn_cast<Instruction>(S0->getPointerOperand());<br>
auto *A1 = dyn_cast<Instruction>(S1->getPointerOperand());<br>
@@ -482,14 +459,14 @@ bool MergedLoadStoreMotion::sinkStore(Ba<br>
assert(S1->getParent() == A1->getParent());<br>
<br>
// New PHI operand? Use it.<br>
- if (PHINode *NewPN = getPHIOperand(BB, S0, S1))<br>
+ if (PHINode *NewPN = getPHIOperand(BB, S0, S1, MD))<br>
SNew->setOperand(0, NewPN);<br>
- removeInstruction(S0);<br>
- removeInstruction(S1);<br>
+ removeInstruction(S0, MD);<br>
+ removeInstruction(S1, MD);<br>
A0->replaceAllUsesWith(ANew);<br>
- removeInstruction(A0);<br>
+ removeInstruction(A0, MD);<br>
A1->replaceAllUsesWith(ANew);<br>
- removeInstruction(A1);<br>
+ removeInstruction(A1, MD);<br>
return true;<br>
}<br>
return false;<br>
@@ -501,7 +478,8 @@ bool MergedLoadStoreMotion::sinkStore(Ba<br>
/// Starting from a diamond tail block, iterate over the instructions in one<br>
/// predecessor block and try to match a store in the second predecessor.<br>
///<br>
-bool MergedLoadStoreMotion::mergeStores(BasicBlock *T) {<br>
+static bool mergeStores(BasicBlock *T, AliasAnalysis *AA,<br>
+ MemoryDependenceResults *MD) {<br>
<br>
bool MergedStores = false;<br>
assert(T && "Footer of a diamond cannot be empty");<br>
@@ -536,8 +514,8 @@ bool MergedLoadStoreMotion::mergeStores(<br>
++NStores;<br>
if (NStores * Size1 >= MagicCompileTimeControl)<br>
break;<br>
- if (StoreInst *S1 = canSinkFromBlock(Pred1, S0)) {<br>
- bool Res = sinkStore(T, S0, S1);<br>
+ if (StoreInst *S1 = canSinkFromBlock(Pred1, S0, AA)) {<br>
+ bool Res = sinkStore(T, S0, S1, MD);<br>
MergedStores |= Res;<br>
// Don't attempt to sink below stores that had to stick around<br>
// But after removal of a store and some of its feeding<br>
@@ -575,8 +553,8 @@ bool MergedLoadStoreMotion::runOnFunctio<br>
// Hoist equivalent loads and sink stores<br>
// outside diamonds when possible<br>
if (isDiamondHead(BB)) {<br>
- Changed |= mergeLoads(BB);<br>
- Changed |= mergeStores(getDiamondTail(BB));<br>
+ Changed |= mergeLoads(BB, AA, MD);<br>
+ Changed |= mergeStores(getDiamondTail(BB), AA, MD);<br>
}<br>
}<br>
return Changed;<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>