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

[Vineet Kumar via Phabricator via llvm-commits]

vkmr added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1499
+/// - For store: Address, stored value, mask, EVL
+class VPPredicatedWidenMemoryInstructionRecipe : public VPRecipeBase {
+  Instruction &Ingredient;
----------------
vkmr wrote:
> 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.
BTW, when you say "extending the existing recipes", do you mean modifying the existing recipe implementation to add a new constructor and EVL and Mask operands or do you mean implementing predicated recipe class as an inherited class from the non-predicated recipe as the base class? 
In my previous reply I assumed the latter, and it is what I prefer over modifying existing recipes (unless it doesn't align with VPlan design principles!) - at the expense of little extra boilerplate code, keeps the two approaches separate and clean.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99750