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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8911
+        Style == TailFoldingStyle::DataAndControlFlowWithoutRuntimeCheck;
+    VPlanTransforms::addActiveLaneMask(*Plan, ForControlFlow,
+                                       WithoutRuntimeCheck);
----------------
Ayal wrote:
> To move all this from tryToBuildVPlanWithVPRecipes() into LVP::optimize() would require moving getTailFoldingStyle(), useActiveLaneMask() et al. from LoopVectorizer.cpp?
Yes, it would require plugging all this through to `optimize()`. Would need to think about how to consolidate passing the required info through in a general way.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:941
+                   [](VPUser *U) { return isa<VPWidenCanonicalIVRecipe>(U); });
+  auto *WideCanonicalIV = R ? cast<VPWidenCanonicalIVRecipe>(*R) : nullptr;
+  VPRecipeBase *LaneMask;
----------------
Ayal wrote:
> 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.
Added an assert that it is in the header. All compares are users of the WideCanonicalIV, the created ALM should be placed to be valid for all of them. Might be good to ensure there's a single VPWidenCanonicalIVRecipe in the future as part of the verifier?


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:970
+    CompareToReplace->replaceAllUsesWith(LaneMask->getVPSingleValue());
+    CompareToReplace->eraseFromParent();
+  }
----------------
Ayal wrote:
> OK to erase a user from parent during traversal over users?
> 
> > Really need a base class for recipes defining a single VPValue....
> +1
Not necessarily, created a temporary vector, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:869
+  auto *CanonicalIVIncrement =
+      cast<VPInstruction>(CanonicalIVPHI->getBackedgeValue());
+  CanonicalIVIncrement->dropPoisonGeneratingFlags();
----------------
Ayal wrote:
> 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.
Thanks, added a comment + TODO


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158779