[llvm] 68490ae - [NFC] Move lambdae into static functions
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 17 10:27:49 PST 2020
Sounds good - thanks for the context!
On Mon, Nov 16, 2020 at 8:54 PM Maxim Kazantsev <mkazantsev at azul.com> wrote:
>
> Hi David,
>
> I've made a set of changes that reused this logic. Initially they also were lambae, but as it grew more and more complex, the size of those lambdae has become comparable with the code outside them. There was a need to hoist out the new functions. But since they used these old lambdae, there were two ways of doing that: either taking std::function parameters, which looked confusing, or hoisting out the used functions as well. I've decided that hoisting out all auxiliary functions as static would make sense to keep the code of the prime user function more compact and readable.
>
> --Max
>
> -----Original Message-----
> From: David Blaikie <dblaikie at gmail.com>
> Sent: Tuesday, November 17, 2020 10:13 AM
> To: Maxim Kazantsev <mkazantsev at azul.com>; Max Kazantsev <llvmlistbot at llvm.org>
> Cc: llvm-commits <llvm-commits at lists.llvm.org>
> Subject: Re: [llvm] 68490ae - [NFC] Move lambdae into static functions
>
> Might be worth having a bit more detail/justification for changes like this. Presumably the author was comfortable with this phrasing - is this change intended to stage these functions for later reuse? Some other purpose?
>
> On Fri, Nov 13, 2020 at 2:08 AM Max Kazantsev via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> >
> >
> > Author: Max Kazantsev
> > Date: 2020-11-13T17:07:25+07:00
> > New Revision: 68490aec4e19d72b9249f3144380f006df23aac5
> >
> > URL:
> > https://github.com/llvm/llvm-project/commit/68490aec4e19d72b9249f31443
> > 80f006df23aac5
> > DIFF:
> > https://github.com/llvm/llvm-project/commit/68490aec4e19d72b9249f31443
> > 80f006df23aac5.diff
> >
> > LOG: [NFC] Move lambdae into static functions
> >
> > Added:
> >
> >
> > Modified:
> > llvm/lib/Transforms/Scalar/IndVarSimplify.cpp
> >
> > Removed:
> >
> >
> >
> > ######################################################################
> > ########## diff --git a/llvm/lib/Transforms/Scalar/IndVarSimplify.cpp
> > b/llvm/lib/Transforms/Scalar/IndVarSimplify.cpp
> > index 4c31064a77da..dfcf89f6d225 100644
> > --- a/llvm/lib/Transforms/Scalar/IndVarSimplify.cpp
> > +++ b/llvm/lib/Transforms/Scalar/IndVarSimplify.cpp
> > @@ -1350,6 +1350,41 @@ analyzeCond(const Loop *L, BranchInst *BI, ScalarEvolution *SE,
> > return CanBeReplacedWithInvariant;
> > }
> >
> > +static void replaceExitCond(BranchInst *BI, Value *NewCond,
> > + SmallVectorImpl<WeakTrackingVH>
> > +&DeadInsts) {
> > + auto *OldCond = BI->getCondition();
> > + BI->setCondition(NewCond);
> > + if (OldCond->use_empty())
> > + DeadInsts.emplace_back(OldCond);
> > +}
> > +
> > +static void foldExit(const Loop *L, BasicBlock *ExitingBB, bool IsTaken,
> > + SmallVectorImpl<WeakTrackingVH> &DeadInsts) {
> > + BranchInst *BI = cast<BranchInst>(ExitingBB->getTerminator());
> > + bool ExitIfTrue = !L->contains(*succ_begin(ExitingBB));
> > + auto *OldCond = BI->getCondition();
> > + auto *NewCond =
> > + ConstantInt::get(OldCond->getType(), IsTaken ? ExitIfTrue :
> > +!ExitIfTrue);
> > + replaceExitCond(BI, NewCond, DeadInsts); }
> > +
> > +static void replaceWithInvariantCond(
> > + const Loop *L, BasicBlock *ExitingBB, ICmpInst::Predicate InvariantPred,
> > + const SCEV *InvariantLHS, const SCEV *InvariantRHS, SCEVExpander &Rewriter,
> > + SmallVectorImpl<WeakTrackingVH> &DeadInsts) {
> > + BranchInst *BI = cast<BranchInst>(ExitingBB->getTerminator());
> > + Rewriter.setInsertPoint(BI);
> > + auto *LHSV = Rewriter.expandCodeFor(InvariantLHS);
> > + auto *RHSV = Rewriter.expandCodeFor(InvariantRHS);
> > + bool ExitIfTrue = !L->contains(*succ_begin(ExitingBB));
> > + if (ExitIfTrue)
> > + InvariantPred = ICmpInst::getInversePredicate(InvariantPred);
> > + IRBuilder<> Builder(BI);
> > + auto *NewCond = Builder.CreateICmp(InvariantPred, LHSV, RHSV,
> > + BI->getCondition()->getName());
> > + replaceExitCond(BI, NewCond, DeadInsts); }
> > +
> > bool IndVarSimplify::optimizeLoopExits(Loop *L, SCEVExpander &Rewriter) {
> > SmallVector<BasicBlock*, 16> ExitingBlocks;
> > L->getExitingBlocks(ExitingBlocks);
> > @@ -1409,38 +1444,6 @@ bool IndVarSimplify::optimizeLoopExits(Loop *L, SCEVExpander &Rewriter) {
> > }
> > #endif
> >
> > - auto ReplaceExitCond = [&](BranchInst *BI, Value *NewCond) {
> > - auto *OldCond = BI->getCondition();
> > - BI->setCondition(NewCond);
> > - if (OldCond->use_empty())
> > - DeadInsts.emplace_back(OldCond);
> > - };
> > -
> > - auto FoldExit = [&](BasicBlock *ExitingBB, bool IsTaken) {
> > - BranchInst *BI = cast<BranchInst>(ExitingBB->getTerminator());
> > - bool ExitIfTrue = !L->contains(*succ_begin(ExitingBB));
> > - auto *OldCond = BI->getCondition();
> > - auto *NewCond = ConstantInt::get(OldCond->getType(),
> > - IsTaken ? ExitIfTrue : !ExitIfTrue);
> > - ReplaceExitCond(BI, NewCond);
> > - };
> > -
> > - auto ReplaceWithInvariantCond = [&](
> > - BasicBlock *ExitingBB, ICmpInst::Predicate InvariantPred,
> > - const SCEV *InvariantLHS, const SCEV *InvariantRHS) {
> > - BranchInst *BI = cast<BranchInst>(ExitingBB->getTerminator());
> > - Rewriter.setInsertPoint(BI);
> > - auto *LHSV = Rewriter.expandCodeFor(InvariantLHS);
> > - auto *RHSV = Rewriter.expandCodeFor(InvariantRHS);
> > - bool ExitIfTrue = !L->contains(*succ_begin(ExitingBB));
> > - if (ExitIfTrue)
> > - InvariantPred = ICmpInst::getInversePredicate(InvariantPred);
> > - IRBuilder<> Builder(BI);
> > - auto *NewCond = Builder.CreateICmp(InvariantPred, LHSV, RHSV,
> > - BI->getCondition()->getName());
> > - ReplaceExitCond(BI, NewCond);
> > - };
> > -
> > bool Changed = false;
> > bool SkipLastIter = false;
> > SmallSet<const SCEV*, 8> DominatingExitCounts; @@ -1450,9 +1453,8
> > @@ bool IndVarSimplify::optimizeLoopExits(Loop *L, SCEVExpander &Rewriter) {
> > // Okay, we do not know the exit count here. Can we at least prove that it
> > // will remain the same within iteration space?
> > auto *BI = cast<BranchInst>(ExitingBB->getTerminator());
> > - auto OptimizeCond = [this, L, BI, ExitingBB, MaxExitCount, &FoldExit,
> > - &ReplaceWithInvariantCond](bool Inverted,
> > - bool SkipLastIter) {
> > + auto OptimizeCond = [this, L, BI, ExitingBB, MaxExitCount, &Rewriter](
> > + bool Inverted, bool SkipLastIter) {
> > const SCEV *MaxIter = MaxExitCount;
> > if (SkipLastIter) {
> > const SCEV *One = SE->getOne(MaxIter->getType()); @@
> > -1463,11 +1465,11 @@ bool IndVarSimplify::optimizeLoopExits(Loop *L, SCEVExpander &Rewriter) {
> > switch (analyzeCond(L, BI, SE, Inverted, MaxIter, InvariantPred,
> > InvariantLHS, InvariantRHS)) {
> > case CanBeRemoved:
> > - FoldExit(ExitingBB, Inverted);
> > + foldExit(L, ExitingBB, Inverted, DeadInsts);
> > return true;
> > case CanBeReplacedWithInvariant: {
> > - ReplaceWithInvariantCond(ExitingBB, InvariantPred, InvariantLHS,
> > - InvariantRHS);
> > + replaceWithInvariantCond(L, ExitingBB, InvariantPred, InvariantLHS,
> > + InvariantRHS, Rewriter,
> > + DeadInsts);
> > return true;
> > }
> > case CannotOptimize:
> > @@ -1511,7 +1513,7 @@ bool IndVarSimplify::optimizeLoopExits(Loop *L, SCEVExpander &Rewriter) {
> > // TODO: Given we know the backedge can't be taken, we should go ahead
> > // and break it. Or at least, kill all the header phis and simplify.
> > if (ExitCount->isZero()) {
> > - FoldExit(ExitingBB, true);
> > + foldExit(L, ExitingBB, true, DeadInsts);
> > Changed = true;
> > continue;
> > }
> > @@ -1533,7 +1535,7 @@ bool IndVarSimplify::optimizeLoopExits(Loop *L, SCEVExpander &Rewriter) {
> > // one?
> > if (SE->isLoopEntryGuardedByCond(L, CmpInst::ICMP_ULT,
> > MaxExitCount, ExitCount)) {
> > - FoldExit(ExitingBB, false);
> > + foldExit(L, ExitingBB, false, DeadInsts);
> > Changed = true;
> > continue;
> > }
> > @@ -1543,7 +1545,7 @@ bool IndVarSimplify::optimizeLoopExits(Loop *L, SCEVExpander &Rewriter) {
> > // exiting iteration, but (from the visit order) strictly follows another
> > // which does the same and is thus dead.
> > if (!DominatingExitCounts.insert(ExitCount).second) {
> > - FoldExit(ExitingBB, false);
> > + foldExit(L, ExitingBB, false, DeadInsts);
> > Changed = true;
> > continue;
> > }
> >
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list