[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/LoopVectorizationPlanner.h:140
 
+  VPInstruction *createOverflowingOp(unsigned Opcode,
+                                     std::initializer_list<VPValue *> Operands,
----------------
Ayal wrote:
> All callers of createOverflowingOp() below pass {false, false} as their WrapFlags, i.e., both HasNUW and HasNSW are set to false, which is the default. Suffice to call createNaryOp() instead?
The current flag-handling code assumes that flags are only queried when the correct `OperationType` is set. Using `createNaryOp` would not set the operation type, so during recipe executing an assert would be triggered when `hasNUW` is called. The main motivation for strict checking is to ensure consistent handling. It effectively enforces that hasNUW is only used on recipes/opcodes that have associated flags, which we would have to weaken, either by having `hasNUW` return false on mismatch or adding extra checks for the operation type. The current handling is similar to how it is handled in LLVM IR.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8911
+        Style == TailFoldingStyle::DataAndControlFlowWithoutRuntimeCheck;
+    VPlanTransforms::addActiveLaneMask(*Plan, ForControlFlow,
+                                       WithoutRuntimeCheck);
----------------
Ayal wrote:
> fhahn wrote:
> > 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.
> Worth a TODO to wrap this all within a `VPlanTransforms::addActiveLaneMask(*Plan)`?
added, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:914
+  VPRecipeBase *OriginalTerminator = EB->getTerminator();
+  Builder.setInsertPoint(EB, OriginalTerminator->getIterator());
+  auto *InLoopIncrement =
----------------
Ayal wrote:
> nit: should be simply `Builder.setInsertPoint(OriginalTerminator);` by introducing `VPBuilder::setInsertPoint(VPRecipeBase*);`
Done, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:939
+
+  auto R = find_if(Plan.getCanonicalIV()->users(),
+                   [](VPUser *U) { return isa<VPWidenCanonicalIVRecipe>(U); });
----------------
Ayal wrote:
> ?
Done, thanks!


================
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:
> 
done ,thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:947
+  } else {
+    if (!WideCanonicalIV)
+      return;
----------------
Ayal wrote:
> fhahn wrote:
> > 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!
> assert still needs to be added, above?
Removed, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:970
+    CompareToReplace->replaceAllUsesWith(LaneMask->getVPSingleValue());
+    CompareToReplace->eraseFromParent();
+  }
----------------
Ayal wrote:
> fhahn wrote:
> > 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!
> Can alternatively avoid erasing from parent here and leave it to dead recipe elimination.
Currently we only run dead recipe removal after induction optimizations, so leaving the compares around keeps the wide induction live. Could be addressed by running dead recipe removal at the beginning of the VPlan optimizations as well separately.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:869
+  auto *CanonicalIVIncrement =
+      cast<VPInstruction>(CanonicalIVPHI->getBackedgeValue());
+  CanonicalIVIncrement->dropPoisonGeneratingFlags();
----------------
Ayal wrote:
> fhahn wrote:
> > 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
> TODO to revisit poison droppings still needs to be added?
added, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:814
+
+// Add a VPActiveLaneMaskPHIRecipe to \p Plan.
+static VPActiveLaneMaskPHIRecipe *
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > This adds much more than a VPActiveLaneMaskPHIRecipe. One part adds recipes in preheader and this phi, returning it for further/any use. Another part replaces the loop branch with several recipes including an ActiveLaneMask. The two parts are apparently independent of each other.
> > Clarified the name & comment. The branch part only adds a Not and Branch recipe, so not sure if it is worth splitting this off?
> Worth explaining what is returned.
> 
> Worth sketching the Inc-for-part(StartV)/ALM at preheader plus Inc-for-part(CanInc/CanPhi)/ALM both feeding ALM-Phi, the latter also feeding Not/BrOnCond which replaces BrOnCount.
> 
> Worth mentioning that StartV/CanPhi/CanInc and their users remain intact, only BrOnCount is replaced, and ALM-phi is returned.
Thanks, expanded comment!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158779