[PATCH] D158779: [VPlan] Add active-lane-mask as VPlan-to-VPlan transformation.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8911
+        Style == TailFoldingStyle::DataAndControlFlowWithoutRuntimeCheck;
+    VPlanTransforms::addActiveLaneMask(*Plan, ForControlFlow,
+                                       WithoutRuntimeCheck);
----------------
To move all this from tryToBuildVPlanWithVPRecipes() into LVP::optimize() would require moving getTailFoldingStyle(), useActiveLaneMask() et al. from LoopVectorizer.cpp?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:941
+                   [](VPUser *U) { return isa<VPWidenCanonicalIVRecipe>(U); });
+  auto *WideCanonicalIV = R ? cast<VPWidenCanonicalIVRecipe>(*R) : nullptr;
+  VPRecipeBase *LaneMask;
----------------
Look for a user U that is a VPWidenCanonicalIVRecipe and resides in the header?
(There could also be multiple such U's....)
As we're considering multiple ICMP_ULE's, they could be using multiple R's, and both could spread out of the header.

Or simply create a WideCanonicalIV before the first non-phi of the header.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:947
+  } else {
+    if (!WideCanonicalIV)
+      return;
----------------
This should be an assert, before checking if UseActiveLaneMaskForControlFlow, because addActiveLaneMask() is called only when tail is folded, which implies an ICMP_ULE pattern.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:970
+    CompareToReplace->replaceAllUsesWith(LaneMask->getVPSingleValue());
+    CompareToReplace->eraseFromParent();
+  }
----------------
OK to erase a user from parent during traversal over users?

> Really need a base class for recipes defining a single VPValue....
+1


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:869
+  auto *CanonicalIVIncrement =
+      cast<VPInstruction>(CanonicalIVPHI->getBackedgeValue());
+  CanonicalIVIncrement->dropPoisonGeneratingFlags();
----------------
fhahn wrote:
> Ayal wrote:
> > nit: worth commenting why poison generating flags are dropped from CanonicalIVIncrement.
> IIUC we should only need to drop them for `DataAndControlFlowWithoutRuntimeCheck`? Might be good to move the code as follow-up and document then.
Worth leaving some note behind for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158779