[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:8951
+  bool HasNUW = true;
+  addCanonicalIVRecipes(*Plan, Legal->getWidestInductionType(), true,
+                        DebugLoc());
----------------



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:869
+  auto *CanonicalIVIncrement =
+      cast<VPInstruction>(CanonicalIVPHI->getBackedgeValue());
+  CanonicalIVIncrement->dropPoisonGeneratingFlags();
----------------
nit: worth commenting why poison generating flags are dropped from CanonicalIVIncrement.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:956
+                Plan, DataAndControlFlowWithoutRuntimeCheck)
+          : nullptr;
+
----------------
Instead of setting to nullptr, create an ALM instruction following canonical IV widened/incrementedForPart placing both before first non-phi of loop header, to make sure the former can serve all potential users of  ICMP_ULE's?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:961
+  // active-lane-mask.
+  VPValue *BTC = Plan.getOrCreateBackedgeTakenCount();
+  vp_for_all_users(BTC, [&](VPUser *U) {
----------------
Only immediate users of BTC are sought, suffice to traverse them via 
`  for (VPUser &U : BTC->users()) {`
rather than
`  vp_for_all_users() {` ?



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:968
+        !isa<VPWidenCanonicalIVRecipe>(CompareToReplace->getOperand(0)) ||
+        CompareToReplace->getOperand(1) != BTC) {
+      return;
----------------
nit: can assert operand 1 is BTC, given that operand 0 is not and the 2-operand compare uses BTC.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:976
+      LaneMask = new VPInstruction(VPInstruction::ActiveLaneMask,
+                                   {WideCanonicalIV, Plan.getTripCount()},
+                                   nullptr, "active.lane.mask");
----------------
The first operand of ALM when addVPLaneMaskPhiAndUpdateExitBranch() generates it in
 - preheader is a `CanonicalIVIncrementForPart(StartV)`
 - latch is `CanonicalIVIncrementForPart(CanonicalIVPHI/CanonicalIVIncrement)`
When generating it here in header it is `VPWidenCanonicalIVRecipe(CanonicalIVPHI)`.

Can ALM use CanonicalIVIncrementForPart(CanonicalIVPHI) here too, for consistency?

Are VPWidenCanonicalIVRecipe and CanonicalIVIncrementForPart interchangeable, are both needed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158779