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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
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.
----------------
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? 


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1370
+/// VPred intrinsics.
+class VPWidenEVLRecipe : public VPRecipeBase, public VPValue {
+
----------------
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.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1499
+/// - For store: Address, stored value, mask, EVL
+class VPPredicatedWidenMemoryInstructionRecipe : public VPRecipeBase {
+  Instruction &Ingredient;
----------------
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?


================
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
----------------
nit: not needed, same for the `zext i32 %N to i64`, you can pass `N` as i64 directly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99750