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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:220
+//
+PHINode *Loop::reverseInductionVariable(ScalarEvolution &SE) {
+  BasicBlock *Preheader = getLoopPreheader();
----------------
fhahn wrote:
> SjoerdMeijer wrote:
> > 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?  
> > 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.
> 
> 
> Right, given that it is only used there, one relatively straight-forward way to handling this might be to add a dedicated VPValue (`%vp.primaryiv`) for the primary induction variable to the plan and use it there. I've put up a patch for that in D77577. I think it might be a beneficial refactor either way, as using Legal->getPrimaryInduction() unnecessarily couples Legal and codegen.
> 
> To use the re-written induction variable, you could just use  something like `State.PrimaryIV = OrigLoop->reverseInductionVariable(*ILV.PSE.getSE());`
> 
> Currently your `reverseInductionVariable` only rewrites the induction variable and related checks, but leaves other users of the IV untouched. I guess that's definitely fine if there are no other users (as in your test cases).  
> Given that I think you also might be able to do the re-write in-place (updating the existing add/icmp/sub instructions instead of creating new ones). Then there would be no need to update the existing VPlan at the moment I think.
> 
> If there are other users, it would mean that we also might change the order of the some operations in the loop (e.g. the order in which memory locations are accessed). This could be avoided by rewriting the uses of the IV with a new expression.
> 
> 
> > 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 think we *don't* want to reverse the IV before deciding to vectorize.
> 
> 
> Right, given that it is only used there, one relatively straight-forward way to handling this might be to add a dedicated VPValue (%vp.primaryiv) for the primary induction variable to the plan and use it there. I've put up a patch for that in D77577. I think it might be a beneficial refactor either way, as using Legal->getPrimaryInduction() unnecessarily couples Legal and codegen.

VPRecipeBuilder::createBlockInMask() is part of VPlan construction rather than codegen, so having it call Legal should be fine. D77635 which follows approach (1) above continues to rely on an original primary induction (start=0, step=1) and its widening, when available, and otherwise widens the new induction created during codegen to control the vector loop (start=0, step=VF*UF).


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

https://reviews.llvm.org/D76838