[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/LoopVectorizationPlanner.h:140
 
+  VPInstruction *createOverflowingOp(unsigned Opcode,
+                                     std::initializer_list<VPValue *> Operands,
----------------
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?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8911
+        Style == TailFoldingStyle::DataAndControlFlowWithoutRuntimeCheck;
+    VPlanTransforms::addActiveLaneMask(*Plan, ForControlFlow,
+                                       WithoutRuntimeCheck);
----------------
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)`?


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


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:941
+                   [](VPUser *U) { return isa<VPWidenCanonicalIVRecipe>(U); });
+  auto *WideCanonicalIV = R ? cast<VPWidenCanonicalIVRecipe>(*R) : nullptr;
+  VPRecipeBase *LaneMask;
----------------



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:947
+  } else {
+    if (!WideCanonicalIV)
+      return;
----------------
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?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:970
+    CompareToReplace->replaceAllUsesWith(LaneMask->getVPSingleValue());
+    CompareToReplace->eraseFromParent();
+  }
----------------
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.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:869
+  auto *CanonicalIVIncrement =
+      cast<VPInstruction>(CanonicalIVPHI->getBackedgeValue());
+  CanonicalIVIncrement->dropPoisonGeneratingFlags();
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158779