[PATCH] D132458: [LoopVectorize] Synthesize mask operands for vector variants as needed

[David Sherwood via Phabricator via llvm-commits]

david-arm added a comment.

Thanks a lot for addressing all the comments @huntergr! I just have a few more minor comments then I think it's good to go. :)



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8402
+
+      VFShape Shape = VFShape::get(*CI, VariantVF, true);
+      unsigned MaskPos = 0;
----------------
nit: Could you change this to

 VFShape Shape = VFShape::get(*CI, VariantVF,  /*HasGlobalPred=*/true);


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8380
         // version of the instruction.
+        if (Variant)
+          return false;
----------------
huntergr wrote:
> david-arm wrote:
> > Doesn't this mean we may end up picking the least optimal VF? For example, if there are v2i32 and v4i32 masked variants we'll only ever pick the v2i32, i.e. the lowest VF?
> No. Since we now store the pointer to the Function in the recipe, we need to force vplan to generate different plans for each VF that has a vector variant available.
> 
> See the vplan checks for 'test_v2_v4m' in synthesize-mask-for-call.ll -- there are separate VF=2 and VF=4 plans, with a widened call to different functions.
OK, can you add some comments here explaining why you are forcing the creation of a new vplan for every subsequent VF after discovering a vector variant?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:934
   Intrinsic::ID VectorIntrinsicID;
+  Function *Variant;
 
----------------
Can you add some comments explaining what this is please? For example, that there should be one recipe for every VF because the variant requires a 1:1 mapping with the VF?


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/synthesize-mask-for-call.ll:8
+
+;; Given the choice between a masked and unmasked variant for the same VF (4)
+;; where no mask is required, make sure we choose the unmasked variant.
----------------
I think it's also worth having a test for the case when both VF=2 and VF=4 use unmasked variants, because even in this case we must create separate vplans for each. The `Variant` member added to the recipe requires it now I think.


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

https://reviews.llvm.org/D132458