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

[Xinliang David Li via llvm-commits]

On Fri, Aug 12, 2016 at 10:11 AM, Dehao Chen <danielcdh at gmail.com> wrote:
>
>
> 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).
>


 right :)

David

> 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
>> >
>> >
>> >
>
>