[PATCH] D99750: [LV, VP] RFC: VP intrinsics support for the Loop Vectorizer (Proof-of-Concept)

[Vineet Kumar via Phabricator via llvm-commits]

vkmr added a comment.

Thanks @fhahn for a look at the VPlan bits of the patch.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3045
         auto *VecPtr = CreateVecPtr(Part, State.get(Addr, VPIteration(0, 0)));
-        if (isMaskRequired)
+        // if EVLPart is not null, we can vectorize using predicated
+        // intrinsic.
----------------
fhahn wrote:
> Do we gain a lot of code-reuse by adding this to `::vectorizeMemoryInstruction`? Seems like it would be cleaner to handle predicated codegen in a separate function? 
I am not too happy about not having a separate function for `::vectorizeMemoryInstruction`! But there is substantial code overlap. Perhaps a better approach would be to abstract out much of the shared code in a separate function and duplicate some parts in a separate `vectorizePredicatedMemoryInstruction`.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1370
+/// VPred intrinsics.
+class VPWidenEVLRecipe : public VPRecipeBase, public VPValue {
+
----------------
fhahn wrote:
> IIUC codegen depends on the induction and trip count for targets with active vector length? Those should probably be modeled as operands for the recipe.
> 
> For the non-active-vector length case, the recipe is loop invariant, right? Could we just use live-in VPValue in those cases instead of the recipe? We may have to set up the value outside the loop for scalable VFs.
> 
> It would also be good to expand the comment to describe the behavior.
> IIUC codegen depends on the induction and trip count for targets with active vector length? Those should probably be modeled as operands for the recipe.

Correct. Given that `TripCount` and `Induction` are members of ILV,  from VPlan's perspective does modeling them as operands for the recipe give any functional benefit or is it more of a VPlan design principle thing?

> For the non-active-vector length case, the recipe is loop invariant, right? Could we just use live-in VPValue in those cases instead of the recipe? We may have to set up the value outside the loop for scalable VFs.

Right. It is the runtime VF of the vector loop. Just to make sure, in the VPlan terminology, a "live-in VPValue" is a direct mapping from an LLVM IR Value defined outside the loop body that is used in the loop body, right?

For scalable vectors , yes we would have to create  an IR instruction for `vscale x vf`. IIUC, there's supposed to be a VPlan built for each possible VF, but it wouldn't be desirable to create an IR instruction for each VF; Does VPlan allow creating a VPValue (that is not a recipe) that would be expand to an IR instruction on plan execution? (Pardon me if I am missing something obvious here!)








================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1499
+/// - For store: Address, stored value, mask, EVL
+class VPPredicatedWidenMemoryInstructionRecipe : public VPRecipeBase {
+  Instruction &Ingredient;
----------------
fhahn wrote:
> Not sure how well mirroring predicated recipes will scale as more recipes get added. Did you consider extending the existing recipes to take an extra operand for the EVL?
I am actually considering and would prefer to extend the existing recipes to support EVL, for the reason you mentioned.

IIRC at the time of our downstream implementation, the existing recipes were different enough and in a state of flux, that it felt cleaner to have mirrored recipes (besides we needed just 2 of them). Doesn't make much sense now, though.


================
Comment at: llvm/test/Transforms/LoopVectorize/vectorize-vp-intrinsics.ll:267
+entry:
+  %cmp10 = icmp sgt i32 %N, 0
+  br i1 %cmp10, label %for.body.preheader, label %for.cond.cleanup
----------------
fhahn wrote:
> nit: not needed, same for the `zext i32 %N to i64`, you can pass `N` as i64 directly.
Good catch!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99750