[PATCH] D125301: [LoopVectorize] Add option to use active lane mask for loop control flow

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:717
     auto *PredTy = VectorType::get(Int1Ty, State.VF);
+
     Instruction *Call = Builder.CreateIntrinsic(
----------------
nit: stray whitespace.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:941
+      (Term->getOpcode() == VPInstruction::BranchOnCount ||
+       Term->getOpcode() == VPInstruction::BranchOnCond) &&
       isa<ConstantInt>(TripCountV)) {
----------------
Is this covered by a test? The comment and logic above applies specifically to `BranchOnCount`. It is not entirely clear to me  that it would be safe for any `BranchOnCond`?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1573
+  printAsOperand(O, SlotTracker);
+  O << " = ACTIVE-LANE-MASK-PHI";
+}
----------------
Should this also print the operands here?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1552
+                        &*State.CFG.PrevBB->getFirstInsertionPt());
+    EntryPart->addIncoming(StartMask, VectorPH);
+    EntryPart->setDebugLoc(DL);
----------------
david-arm wrote:
> fhahn wrote:
> > david-arm wrote:
> > > Hi @fhahn, if I use VPWidenPHIRecipe instead it's not obvious how I add the incoming start mask. It looks like VPWidenPHIRecipe::execute doesn't add it - do you know how?
> > I think D128937 should do the trick, together with the snippet below. If D128937 allows `VPWidenPHIRecipe` to be used here I'll send it for review.
> > 
> > ```
> > diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
> > index d8fb7a9c12b0..1ba76bf52cc6 100644
> > --- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
> > +++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
> > @@ -3646,8 +3646,7 @@ void InnerLoopVectorizer::fixVectorizedLoop(VPTransformState &State,
> >      truncateToMinimalBitwidths(State);
> > 
> >    // Fix widened non-induction PHIs by setting up the PHI operands.
> > -  if (EnableVPlanNativePath)
> > -    fixNonInductionPHIs(Plan, State);
> > +  fixNonInductionPHIs(Plan, State);
> > 
> >    // At this point every instruction in the original loop is widened to a
> >    // vector form. Now we need to fix the recurrences in the loop. These PHI
> > ```
> Hi @fhahn, if you're happy with everything else in the patch, would you be ok with submitting this patch as it is now, then look into using VPWidenPHIRecipe as a follow-on piece of refactoring? We're quite keen to get this work submitted soonish so we can get sufficient testing in the run-up to the LLVM 15 release. We also have quite a few other pieces of work on-going that depend upon this too.
Ok sounds good to me, could you please add a FIXME/TODO to the code in the meantime?

> We also have quite a few other pieces of work on-going that depend upon this too.

If other stuff depends on the patch it might be helpful to link them together as patch stack, to get a better idea on the wider impact.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2564
+  /// isn't one, then return nullptr.
+  VPActiveLaneMaskPHIRecipe *getActiveLaneMaskPhi();
+
----------------
What about multiple VPActiveLaneMasks? Should the verifier check to make sure there's only a single one?


It would probably also be good to place this after `getCanonicalIV`, which seems related.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp:193
+    errs() << "VPlan vector loop exit must end with BranchOnCount or "
+              "BranchOnActiveLaneMask VPInstruction\n";
     return false;
----------------
This still references `BranchOnActiveLaneMask`?


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

https://reviews.llvm.org/D125301