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