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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
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;
+  }
+
----------------
Ayal wrote:
> ?
Simplified as suggested, thanks!


================
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)) {
----------------
Ayal wrote:
> 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?
I'll share a patch soon to add active-lane-mask as optimization.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8063
+                                     nullptr, "active.lane.mask");
+  } else {
+    VPValue *BTC = Plan.getOrCreateBackedgeTakenCount();
----------------
Ayal wrote:
> 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.
I'll share a patch soon to add active-lane-mask as optimization.



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


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8822
 
       RecipeBuilder.setRecipe(Instr, Recipe);
+      if (isa<VPHeaderPHIRecipe>(Recipe)) {
----------------
Ayal wrote:
> 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.
Sounds good but I think it would be better to do that separately from the current change.


================
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
----------------
Ayal wrote:
> nit: seems slightly more logical to place createHeaderMask() first, before createBlockInMask(), which in turn continues to be followed by createEdgeMask.
Yep, reordered.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157037