[PATCH] D135017: [LV] Move exit cond simplification to separate transform.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 4 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7623
+  if (!IsEpilogueVectorization)
+    VPlanTransforms::optimizeForConcreteVFAndUF(BestVPlan, BestVF, BestUF, PSE);
+
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > nit: can drop "Concrete".
> > > 
> > > Restrict the VF range of BestPlan to include only BestVF?
> > > 
> > > When printing BestPlan would be good to now indicate that it applies to BestUF only.
> > > nit: can drop "Concrete".
> > 
> > Done!
> > 
> > > Restrict the VF range of BestPlan to include only BestVF?
> > 
> > Done!
> > 
> > > When printing BestPlan would be good to now indicate that it applies to BestUF only.
> > 
> > Should I add a field for that?
> >> When printing BestPlan would be good to now indicate that it applies to BestUF only.
> >
> > Should I add a field for that?
> 
> Probably, to then print it instead of "UF >= 1". Can also add setUF(), and an assert when executing (the epilog loop) that the plan used works for the current UF.
Thanks, I landed the following improvements separately:

* move generating of VFs & UFs as part of `getName` to ensure it always prints the up-to-date ones: 96296922b61f
* tracking of UF in VPlan: 5df34e971dd5
* add asserts in executePlan that plan has selected VF + UF: b7b1e5c96fe0


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:471
+  auto *Term = dyn_cast<VPInstruction>(&ExitingVPBB->back());
+  // Try to simplify the branch condition if BTC < VF * UF when preparing to
+  // execute the plan for the main vector loop. We only do this if the
----------------
Ayal wrote:
> So we retain "if TC <= VF * UF" ?
Yes, updated!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:483
+      Plan.getCanonicalIV()->getStartValue()->getLiveInIRValue()->getType();
+  const SCEV *ExitCount = createTripCountSCEV(IdxTy, PSE);
+  auto *C = dyn_cast<SCEVConstant>(ExitCount);
----------------
Ayal wrote:
> nit: ExitCount >> TripCount ?
> 
> nit: where is createTripCountSCEV defined? Should VPlan or its loop Region provide it, similar to providing the canonical IV, or even have the canonical IV recipe provide it? When optimizing the loop branch recipe, one can also optimize the canonical IV recipe...
> nit: ExitCount >> TripCount ?

Fixed, thanks!


> nit: where is createTripCountSCEV defined?
At the moment it is a regular function, as it is also used outside Plan. But once the scaffolding-generation is also handled by VPlan it could be moved there.




================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:486
+  ScalarEvolution &SE = *PSE.getSE();
+  if (!C || ExitCount->isZero() ||
+      C->getAPInt().getZExtValue() > BestVF.getKnownMinValue() * BestUF)
----------------
Ayal wrote:
> nit: is the isZero() check needed?
Yes for now I think; if it is zero then the `+1` overflowed and this check is the same as in the previous version of the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135017