[PATCH] D22778: Add Loop Sink pass to reverse the LICM based of basic block frequency.

[Dehao Chen via llvm-commits]

On Fri, Aug 12, 2016 at 9:48 AM, Xinliang David Li <davidxl at google.com>
wrote:

> On Thu, Aug 11, 2016 at 11:13 PM, Dehao Chen <danielcdh at gmail.com> wrote:
> > danielcdh added inline comments.
> >
> > ================
> > Comment at: lib/Transforms/Scalar/LoopSink.cpp:122
> > @@ +121,3 @@
> > +  // Compute alias set.
> > +  for (BasicBlock *BB : L->blocks())
> > +    CurAST.add(*BB);
> > ----------------
> > davidxl wrote:
> >> This is not correct -- it should merge in SubLoop's AST too. See LICM's
> CollectAliasInfoForLoop -- that should be extracted as a common utility.
> > Yes, we can refactor the code to reuse the CollectAliasInfoForLoop from
> LICM for legacy passmanager:
> >
> > 1. Build a base class ASTLoopTransformation, and make
> LookInvariantCodeMotion and LoopSink sub class of it. LoopToAliasSetMap and
> collectAliasInfoForLoop should be protected member of the base class. The
> logic in the sub class will also be shared with the new pass manager.
> > 2. Build a base class ASTLoopLegacyPass which inherits from LoopPass,
> and LICMLegacyPass and LoopSinkLegacyPass subclass of it.
> cloneBasicBlockAnalysis, deleteAnalysisValue and deleteAnalysisLoop should
> be private member of the base class. Base class also need to have a
> ASTLoopTransformation pointer to invoke the actual logic shared with new
> pass manager.
> >
> > This is definitely doable, but seems an over kill to me because:
> > 1. collectAliasInfoForLoop is an optimization for compile time. And it
> is not yet available in the new pass manager.
> > 2. The refactoring mainly focuses on abstraction of the old pass
> manager, which will be replaced by new pass manager soon.
> > 3. There is much complexity involved because new pass manage does not
> support this optimization, and we need to make it fall back to what we do
> right now (add all basic blocks to AST) without introducing memory leak.
> >
> > Comments?
>
> Ok -- I think it is fine to have more sharing here once new PM support
> for that is ready. For now,  I think you still need to add subloop's
> pointers.
>

L->blocks() will iterate all blocks including those belonging to sub loops.
If we need to find blocks that only belongs to the parent loop, we need to
check if (LI->getLoopFor(BB) == L).

Dehao


>
>
> >
> > ================
> > Comment at: lib/Transforms/Scalar/LoopSink.cpp:131
> > @@ +130,3 @@
> > +    Instruction &I = *II++;
> > +    if (L->hasLoopInvariantOperands(&I) && canSinkToLoopBody(I, AA, L,
> &CurAST))
> > +      INS.push_back(&I);
> > ----------------
> > davidxl wrote:
> >> Is the first check needed?
> > I think yes because if there is loop variant in its operand, sinking it
> into the loop may change the value every iteration.
> >
> > ================
> > Comment at: lib/Transforms/Scalar/LoopSink.cpp:155
> > @@ +154,3 @@
> > +        if (BFI->getBlockFreq(CDT) >= PreheaderFreq ||
> > +            BFI->getBlockFreq(CDT) *
> > +                    BranchProbability(SinkFrequencyPercentThreshold,
> 100) >
> > ----------------
> > davidxl wrote:
> >> This logic seems to be inverted -- Using CDT should be encouraged if
> its frequency equals the sum of SinkBB and N. In other words, division
> should be used, not multiplication
> > It's actually expected: if CDT's frequency is equal or only a little
> larger than SinkBB+N's frequency, then the check will fail and goes to
> "else" branch: picking the CDT instead SinkBB.
> >
>
> Ok.
>
>
> David
>
>
> > ================
> > Comment at: lib/Transforms/Scalar/LoopSink.cpp:183
> > @@ +182,3 @@
> > +    }
> > +    if (ShouldSkip || T > PreheaderFreq ||
> > +        (SinkBBs.size() > 1 &&
> > ----------------
> > davidxl wrote:
> >> Is the formatting correct here?
> > I used clang-format --style=llvm for the formatting.
> >
> >
> > https://reviews.llvm.org/D22778
> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160812/e3853598/attachment.html>