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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8156
+  if (OrigLoop->getHeader() == BB)
+    return BlockMaskCache[BB];
+
----------------
ABataev wrote:
> It may create a new item in BlockMaskCache with nullptr value. Do you need to check that the user actually expects it?
Agreed. Better explicitly always set BlockMaskCache[] for header BB, and assert BB is not the header after looking for its mask in the cache below.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8164
   // All-one mask is modelled as no-mask following the convention for masked
   // load/store/gather/scatter. Initialize BlockMask to no-mask.
   VPValue *BlockMask = nullptr;
----------------
nit: setting BlockMask to null here mostly represents "uninitialized" - to be overwritten by the first edge mask; and to serve as the (all-true) mask for blocks free of any predecessors.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8203
+  auto *IV = new VPWidenCanonicalIVRecipe(Plan.getCanonicalIV());
+  HeaderVPBB->insert(IV, HeaderVPBB->getFirstNonPhi());
+
----------------
nit (independent of this patch).


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8914-8917
+  // Proactively create header mask when tail-folding. Masks for other blocks
+  // are created on demand.
+  if (CM.foldTailByMasking())
+    RecipeBuilder.createTailFoldHeaderMask(*Plan);
----------------
and have createHeaderMask() set the mask to null if foldTail is false?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8976
+      if (isa<VPHeaderPHIRecipe>(Recipe) &&
           HeaderVPBB->getFirstNonPhi() != VPBB->end()) {
+        // Move VPWidenIntOrFpInductionRecipes to the PHI section if the header
----------------
nit (independent of this patch): can the check that first non phi isn't `end()` be dropped? I.e., inserting before end is fine and equivalent to append.
nit (given longer explanation): consider dealing with the simpler appending case first.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8977
           HeaderVPBB->getFirstNonPhi() != VPBB->end()) {
-        // Move VPWidenIntOrFpInductionRecipes for optimized truncates to the
-        // phi section of HeaderVPBB.
-        assert(isa<TruncInst>(Instr));
+        // Move VPWidenIntOrFpInductionRecipes to the PHI section if the header
+        // block. This is needed either when
----------------
ahh, thanks for the clarification!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8979
+        // block. This is needed either when
+        // * tail-folding (mask recipes will be created before the regular
+        //   induction recipes),
----------------
nit: "before" here means both in time and space. Perhaps "non-phi recipes setting header mask are introduced earlier than regular header phi recipes, and should appear after them," would be clearer. (Related to all header phi's, not only inductions).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157037