[PATCH] D157037: [VPlan] Proactively create mask for tail-folding up-front (NFCI).

[Ayal Zaks via Phabricator via llvm-commits]

Ayal accepted this revision.
Ayal added a comment.
This revision is now accepted and ready to land.

This looks good to me, adding some thoughts that could be addresses now or later.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7997-8007
+  // For the loop header return the cached block mask.
+  if (OrigLoop->getHeader() == BB) {
+    auto Iter = BlockMaskCache.find(BB);
+    assert(Iter != BlockMaskCache.end() && "header mask must be set");
+    return Iter->second;
+  }
+
----------------
?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8040
+  // mask from the active lane mask PHI that is cached in the VPlan.
+  TailFoldingStyle TFStyle = CM.getTailFoldingStyle();
+  if (useActiveLaneMaskForControlFlow(TFStyle)) {
----------------
Would it be better to have one `switch (TFStyle)` whose cases compute BlockMask, followed by placing it in the cache, where `None` refers to not folding the tail?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8063
+                                     nullptr, "active.lane.mask");
+  } else {
+    VPValue *BTC = Plan.getOrCreateBackedgeTakenCount();
----------------
This `else` case of not using active lane mask is aka `DataWithoutLaneMask`.

ActiveLaneMask is semantically equivalent to an ICmpULE (except for wrapping TC?), so down the road we could start canonically with one and if desired replace it with the other, or with an ActiveLaneMaskPhi, or None, as VPlan2VPlan transforms.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8760
       getDebugLocFromInstOrOperands(Legal->getPrimaryInduction());
   addCanonicalIVRecipes(*Plan, Legal->getWidestInductionType(),
                         DLInst ? DLInst->getDebugLoc() : DebugLoc(),
----------------
Sh/Could addCanonicalIVRecipes() move to RecipeBuilder?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8822
 
       RecipeBuilder.setRecipe(Instr, Recipe);
+      if (isa<VPHeaderPHIRecipe>(Recipe)) {
----------------
Would something like the following be better?
```
auto InsertionPoint = isa<VPHeaderPHIRecipe>(Recipe) ? VPBB->getFirstNonPhi() : VBPP->end();
Recipe->insertBefore(*VPBB, InsertionPoint);
```
with an
```
assert ((!isa<VPHeaderPHIRecipe>(Recipe) ||
         VPBB->getFirstNonPhi() == VPBB->end() ||
         CM.foldTailByMasking() || isa<TruncInst>(Instr)) &&
               "unexpected recipe needs moving");
```
?

Note that phi recipes in VPBB's other than header VPBB also need to be placed in the phi section of their VPBB, in general. However, (a) this currently includes only VPPredInstPHIRecipe, which is introduced into VPlan only later, and (b) all current cases of out-of-place recipes occur in the header only - its mask and a trunc of an induction phi.


================
Comment at: llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h:143
+  void createHeaderMask(VPlan &Plan);
+
   /// A helper function that computes the predicate of the edge between SRC
----------------
nit: seems slightly more logical to place createHeaderMask() first, before createBlockInMask(), which in turn continues to be followed by createEdgeMask.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157037