[PATCH] D76838: [LV][LoopInfo] Transform counting-down loops to counting-up loop

[Sam Parker via Phabricator via llvm-commits]

samparker added inline comments.


================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:580
   /// The IndVarSimplify pass transforms loops to have a canonical induction
   /// variable.
   ///
----------------
Ayal wrote:
> SjoerdMeijer wrote:
> > Ayal wrote:
> > > Following the above comment, this Analysis should rely on a previous Transformation to produce a canonical induction variable, if needed.
> > > 
> > > If this transformation is applied to a loop before deciding to vectorize it, there may be potential slowdowns when the loop remains unvectorized; so best handled independent of LV.
> > > 
> > > In terms of implementation, as far as LV is concerned, if getCanonicalInductionVariable() fails and one is to be constructed, do so by relying on  SCEV::getBackedgeTakenCount() instead of pattern matching.
> > > 
> > > Cf. http://lists.llvm.org/pipermail/llvm-dev/2020-March/140433.html
> > Sorry, but I just want to be sure, where does the transformation need to be implemented? Is that Indvar simplify, in SCEV, or a looputil function?
> > 
> > I may have also read a few things differently than I do know. For example, I thought I understood that it was undesired to do this in IndVarSimplify from the mail on the dev list. And also regarding that mail http://lists.llvm.org/pipermail/llvm-dev/2020-March/140433.html, and perhaps I should write this to the list, but I don't think I understand "As a consequence, any loop structure that is recognized by SCEV will (/should) not profit from rewriting."
> >  
> If the transformation is to be applied to all loops, vectorized or not, it should be part of Indvar, as it once was.
> The argument on the dev list is that (a) it has and may still cause slowdowns with small if any profit, and (b) passes should use SCEV instead of relying on specific forms of IR patterns. Tried to argue against both in http://lists.llvm.org/pipermail/llvm-dev/2020-April/140539.html. Perhaps only a limited form of the old iv-rewrite should be re-enabled, e.g., canonicalizing the primary iv only, in loops that "appear" vectorizable.
Considering that this change is just to enable a specific part of the vectorizer to do its thing, I'm not convinced that extracting it out is the way to go, especially when it would cause many more (unnecessary) changes.


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

https://reviews.llvm.org/D76838