[PATCH] D136251: [LoopVectorize] Use available masked vector functions when required

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:285
+    // otherwise we look for one that matches the supplied VF.
+    auto Mappings = VFDatabase::getMappings(CI);
+    for (VFInfo Info : Mappings)
----------------
I wonder if it's useful to have a `VFDatabase::getMaskedMappings` variant or something like that? That way we can avoid walking through all the mappings if there aren't going to be any masked variants anyway. I was thinking about something like this:

  auto Mappings = VFDatabase::getMaskedMappings(CI);
  if (Mappings.emtpy())
    return false;
  for (VFInfo Info : Mappings)
    if (!VF || Info.Shape.VF == *VF)
      return true;
  return false;

Do you think it's worth it?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3435
 
 InstructionCost LoopVectorizationCostModel::getVectorCallCost(
     CallInst *CI, ElementCount VF, bool &NeedToScalarize, Function **Variant,
----------------
Looks like this is based off an old version of D132458. Could you rebase please?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3469
   InstructionCost MaskCost = 0;
-  VFShape Shape = VFShape::get(*CI, VF, false /*HasGlobalPred*/);
+  VFShape Shape = VFShape::get(*CI, VF, Legal->isMaskRequired(CI));
+  if (NeedsMask)
----------------
I think it's worth caching the return value of `Legal->isMaskRequired(CI)` and reusing here, since it's used 3 times and it saves the compiler having to rewalk the `MaskedOp` list in `LoopVectorizationLegality`


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8341
                                                    VFRange &Range,
-                                                   VPlanPtr &Plan) const {
+                                                   VPlanPtr &Plan) {
   bool IsPredicated = LoopVectorizationPlanner::getDecisionAndClampRange(
----------------
Just out of interest why do we need to remove `const`?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8399
+
+      VPValue *Mask = nullptr;
+      if (Legal->isMaskRequired(CI))
----------------
Perhaps it's worth adding some comments here explaining what the two scenarios are, i.e. 1) The blocks needs predication so we have to use the mask for that block, or 2) the block doesn't need predication but we are using a masked variant so need to synthesize an all-true mask.


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

https://reviews.llvm.org/D136251