[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:8156
+  if (OrigLoop->getHeader() == BB)
+    return BlockMaskCache[BB];
+
----------------
Ayal wrote:
> 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.
Thanks, updated to use `.find()` and added an assert. Clients already should handle the case the mask is nullptr, as this is used to indicate no-mask (all-true mask) in the existing code.


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


================
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);
----------------
Ayal wrote:
> and have createHeaderMask() set the mask to null if foldTail is false?
Updated, thanks!


================
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
----------------
Ayal wrote:
> 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.
I dropped the check from the condition in 57a6f6579c97 and moved it to the assert, thanks!


================
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
----------------
Ayal wrote:
> ahh, thanks for the clarification!
Adjusted, thanks!


================
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),
----------------
Ayal wrote:
> 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).
Reworded, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157037