[PATCH] D99750: [LV, VP]VP intrinsics support for the Loop Vectorizer

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9845
+    if (auto *IMask = dyn_cast<VPInstruction>(Mask))
+      if (IMask->getOpcode() == VPInstruction::ICmpULE)
+        return Builder.getTrueVector(EC);
----------------
fhahn wrote:
> ABataev wrote:
> > fhahn wrote:
> > > ABataev wrote:
> > > > fhahn wrote:
> > > > > It's not clear to me what the reasoning is here to simplify this to the all-true mask without checking the operands of the compare. If the compare can be simplified to the all-true mask, then this would be suitable to do as VPlan-to-VPlan simplification instead of during codegen.
> > > > Added extra check.
> > > Thanks for the update. Looking at this again, it seems like it would be better to either perform this simplification when creating the tail-folding mask or optimize as VPlan-to-VPlan transform, as presumably this applies to all users of the top-level mask? Might be good to tie in with D157037 which would always create the top-level mask in the beginning when tail-folding.
> > If I understand your proposal correctly, it will require adding special VPValue like AllTrueVPValue. This lambda returns Value *, no VPValue, so it cannot be done directly without adding new live-in(?).
> I think it would be preferable to handle this more generically during VPlan optimizations.
> 
> I’ve been working on moving tail folding to a Vallant transform instead of combining it with regular block mask creation (D157713)
> 
> With that, this simplification could be handled generically by not adding the header mask during the tail folding transform.
Shall I wait for your patch or we can land this with the current implementation so you can later adjust this in your patch?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:292
+    Value *EVL = HasEVL ? State.get(getOperand(1), Part) : nullptr;
+    if (Part == 0 || EVL) {
       bool IsNUW = getOpcode() == VPInstruction::CanonicalIVIncrementNUW;
----------------
fhahn wrote:
> fhahn wrote:
> > IIUC the `EVL` check here is not needed? Can have an asset that `EVL` implies `Part == 0`. IIUC Part > 0 could be a problem is that there could be different EVL's for different parts?
> I spent some time thinking on how EVL could
> be modeled more explicitly here and shared D157322 which adds an argument for the increment which could be set to EVL when using it, removing the need to modify the recipe itself here
Yep, we can do it. Same question as before - shall I wait for your patch to be landed or we can commit it and then later you will update шеп in your patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99750