[PATCH] D63921: [Loop Peeling] Add support for peeling of loops with multiple exits

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: lib/Transforms/Utils/LoopUnroll.cpp:412
       Preheader = L->getLoopPreheader();
       ULO.TripCount = SE->getSmallConstantTripCount(L, ExitingBlock);
       ULO.TripMultiple = SE->getSmallConstantTripMultiple(L, ExitingBlock);
----------------
skatkov wrote:
> fhahn wrote:
> > skatkov wrote:
> > > fhahn wrote:
> > > > IIRC TripCount and TripMultiple would not be set for multi-exit loops (in LoopUnrollPass.cpp), but MaxTripCount would. But maybe I am missing the reasoning behind choosing the trip count of one of the exits?
> > > It is not only for multi exit but also for single exit.
> > > We support other exits only for deopts, so no need to worry about them.
> > > We support other exits only for deopts, so no need to worry about them.
> > 
> > Right, I missed that restriction. I think it is fine to have this restriction to start with, but it would be good to explicitly mention that here in a comment and in the commit message.
> > 
> > 
> To be honest, I' mot sure what exactly you mean. This patch does not modify the guard to allow loops with multiple exits. with this patch we still support only loops with one exit on latch.
> 
> The patch does modification to not directly depend on the condition that there is only one exit.
> So this particular line change is done because L->getExitingBlock() does not work for multiple exits but we are sure this exiting block is latch and it will be true both for single and multiple exits if others are calls to deopt.
> 
> I will update D63923 to comment here about other exits.
I meant that I think we were missing an explanation here, for why we would always pick the latch (instead of the getExitingBlock()). Anyways, I missed that in a recent update, you added a comment where you use getLoopLatch, which makes it clear. Thanks


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63921/new/

https://reviews.llvm.org/D63921