[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:20:02 PDT 2016
(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)
On Thu, Jun 16, 2016 at 9:07 AM, Daniel Berlin <dberlin at dberlin.org> wrote:
> 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/3076cf6f/attachment.html>
More information about the llvm-commits
mailing list