[PATCH] D143422: [LV] Update logic for calculating register usage due to invariants

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6122
+      }
     }
 
----------------
sushgokh wrote:
> sdesmalen wrote:
> > sdesmalen wrote:
> > > I think you can write the algorithm above a bit shorter using a lambda:
> > > 
> > >   bool IsScalar = all_of(cast<Instruction>(Inst)->users(), [&](User *U) {
> > >     auto *I = dyn_cast<Instruction>(U);
> > >     return !I || TheLoop != LI->getLoopFor(I->getParent()) ||
> > >            isScalarAfterVectorization(I, VFs[i]);
> > >   });      
> > >   
> > >   ElementCount VF = IsScalar ? ElementCount::getFixed(1) : VFs[i];
> > >   unsigned  ClassID = TTI.getRegisterClassForType(VF.isVector(), Inst->getType());
> > >   Invariant[ClassID] += GetRegusage(Inst->getType(), VF);;
> > > 
> > > I think you can write the algorithm above a bit shorter using a lambda:
> > > 
> > >   bool IsScalar = all_of(cast<Instruction>(Inst)->users(), [&](User *U) {
> > >     auto *I = dyn_cast<Instruction>(U);
> > >     return !I || TheLoop != LI->getLoopFor(I->getParent()) ||
> > >            isScalarAfterVectorization(I, VFs[i]);
> > >   });      
> > >   
> > >   ElementCount VF = IsScalar ? ElementCount::getFixed(1) : VFs[i];
> > >   unsigned  ClassID = TTI.getRegisterClassForType(VF.isVector(), Inst->getType());
> > >   Invariant[ClassID] += GetRegusage(Inst->getType(), VF);;
> > > 
> > @sushgokh did you try the above suggestion?
> > 
> > When I try it locally, all the tests in your patch still pass.
> @sdesmalen Yes, I tried your suggestion and all the tests passed. But,my only intention was not to assume anything(as I have stated in the comment). But if you advise, will go with your suggestion
That case should already be covered because in getMaxLegalScalableVF it goes through all instructions in the loop to see if the element type is legal for scalable vectors.
So if there are any uses of this scalar value that are non-scalar, then the code in `getMaxLegalScalableVF` should have prevented the LV from considering a scalable VF for this loop.

Based on that, I believe my suggestion is safe.


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

https://reviews.llvm.org/D143422