[PATCH] D89541: [IRCE] Do not transform is loop has small number of iterations

[Serguei Katkov via Phabricator via llvm-commits]

skatkov added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:1903
+  // Profitability check.
+  if (!SkipProfitabilityChecks && GetBFI.hasValue()) {
+    BlockFrequencyInfo &BFI = (*GetBFI)();
----------------
ebrevnov wrote:
> skatkov wrote:
> > ebrevnov wrote:
> > > There is an existing utility function inside the vectorizer (getSmallBestKnownTC) which estimates loop's trip count. Can this be used (if made public) instead of BFI?
> > The utility you mentioned uses SE and BPI on latch only to estimate the trip count.
> > 
> > IRCE has already estimation basing on BPI of latch. It is not enough if the main exit from the loop is not a latch like in the test I've added to this CL.
> > 
> > May be for vectorizer it is ok to be based on latch but not for IRCE.
> > The utility you mentioned uses SE
> That's right, currently it does use of SE. Since it aims at returning best known TC I don't see any problems to make SE optional.
> 
> >  and BPI on latch only to estimate the trip count.
> In fact, it doesn't use BPI. It checks for profile info directly. I take this another reason to have one source of estimation for loop's TC.
> If BPI available it would be preferable to relay on it instead of profile info. If BFI available, there is no need to use BPI since it is completely based on BPI. If we know exact trip count from SE know need to use BFI.
> 
> > 
> > IRCE has already estimation basing on BPI of latch. It is not enough if the main exit from the loop is not a latch like in the test I've added to this CL.
> If the above suggestion looks too complex I would still want us to merge the logic at least inside IRCE. If BFI is available I don't see why we should even try to do an estimation based on BPI for the latch inside parseLoopStructure.
> 
> > 
> > May be for vectorizer it is ok to be based on latch but not for IRCE.
> 
> 
I'll do a follow-up patch for this.


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

https://reviews.llvm.org/D89541