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

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer marked an inline comment as done.
SjoerdMeijer added inline comments.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:220
+//
+PHINode *Loop::reverseInductionVariable(ScalarEvolution &SE) {
+  BasicBlock *Preheader = getLoopPreheader();
----------------
Ayal wrote:
> fhahn wrote:
> > SjoerdMeijer wrote:
> > > Ayal wrote:
> > > > SjoerdMeijer wrote:
> > > > > Meinersbur wrote:
> > > > > >  `Loop` is part of the `LLVMAnalysis` library. Transformations should be in the `LLVMTransform` library.
> > > > > > 
> > > > > > Also, it is untypical for the analysis results to have methods for modification as well. These are typically found in the LLVMTransform library such as `LoopUtils.cpp`
> > > > > Thanks for the suggestion, I'm happy to move this to LoopUtils.cpp. Also checking with @Ayal, is this something we can all live with? 
> > > > Constructing a canonical induction if one is missing and needed for LV's FoldTail, using SCEV::getBackedgeTakenCount() instead of pattern matching, could be placed in LoopUtils.cpp (or in LoopVectorize.cpp, using ILV::createInductionVariable()).
> > > > 
> > > > The concern is that doing so might cause slowdowns if the loop is not vectorized, something LV has been trying hard to avoid, so far successfully, which also motivated VPlan. (BTW, doing so may also cause slowdowns even if the loop is vectorized, but that's a different story ;-). Reverting control to the original IV if the loop is not vectorized seems awkward at best.
> > > > 
> > > > Let's try to think of alternative (1), i.e., of having LV represent in VPlan the new PIV that it will eventually create. A new PrimaryInductionRecipe  (or VPInstruction) can be introduced and placed at the beginning of the first VPBasicBlock; its execute() will create a Phi-starting-at-zero, set ILV::Induction and possibly a PIV VPValue to it; the bump and compare could be taken care of in ILV.fixVectorizedLoop().
> > > > Interested in following this approach?
> > > Thanks for the suggestion Ayal. My initial thought was that this sounds like a lot of different moving parts for a simple thing like this. But if it overcomes the problem of doing this transformation unnecessary, then that sounds like a good plan. And also, creating a vplan recipe migt not be that bad, i.e. actually a good fit for this. I need to get up to speed with how the vplan machinery works, which is what I am doing first at the moment. 
> > IIUC to solve the issue, we have to 1) check if we have an induction variable we can reverse in LVL, 2) record that, 3) reverse the IV when generating code.
> > 
> > I might be missing something, but wouldn't it be easiest to record IVs that require reversing in LVL (similar to how we already record IVs and reductions) and use the reversing utility as a preparation step at codegen time?
> > 
> > At the moment, the def/use chains are not yet modeled in VPlan completely, so introducing a new recipe would not add a lot of value (I might be missing something though), as we cannot update the users in the VPlan to use the reversed IV.
> > 
> > Unless I am missing something, the issue could be addressed easiest by recording the information in LVL and reverse the induction variable using the utility as a codegen preparation step before executing the VPlan. Once the def/use chain is migrated to VPlan, it should be straight-forward to handle the case in VPlan.
> When building VPlan with FoldMask, a `VPValue *IV` of the Primary Induction Variable is needed by
> 
> ```
>     // Introduce the early-exit compare IV <= BTC to form header block mask.
>     // This is used instead of IV < TC because TC may wrap, unlike BTC.
>     VPValue *IV = Plan->getVPValue(Legal->getPrimaryInduction());
>     VPValue *BTC = Plan->getOrCreateBackedgeTakenCount();
>     BlockMask = Builder.createNaryOp(VPInstruction::ICmpULE, {IV, BTC});
> ```
> Approach (2) is to fix the incoming IR so it has a PIV, approach (1) is to represent the PIV using ingredient-less VPValue/Recipe.
Hi Florian,

> IIUC to solve the issue, we have to 1) check if we have an induction variable we can reverse in LVL, 2) record that, 3) reverse the IV when generating code.

Yep, that's pretty much it. And just to add a little bit to your point 3: generate the code just before the vectoriser does most of its analysis/transformations.

> I might be missing something, but wouldn't it be easiest to record IVs that require reversing in LVL (similar to how we already record IVs and reductions) and use the reversing utility as a preparation step at codegen time?

Now, it's my turn to double-check something :), do you mean like it is effectively done here in this patch? I mean, some details might need some work, code might need to be moved around a bit, but is this not essentially what this patch is doing? Or did you have something fundamentally different in mind?  


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

https://reviews.llvm.org/D76838