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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a reviewer: Ayal.
fhahn added inline comments.


================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:537
   /// intrinsic is supported.
-  bool emitGetActiveLaneMask() const;
+  PredicationStyle emitGetActiveLaneMask() const;
 
----------------
Looks like this comment needs updating, as the interface is more fine grained now?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8712
+
+    auto *ALM = new VPInstruction(VPInstruction::ActiveLaneMask,
+                                  {CanonicalIVIncrementParts, TC}, DL);
----------------
fhahn wrote:
> david-arm wrote:
> > fhahn wrote:
> > > david-arm wrote:
> > > > fhahn wrote:
> > > > > david-arm wrote:
> > > > > > fhahn wrote:
> > > > > > > It looks like the update for the phi doesn't depend on the phi, but only on trip count & main induction. 
> > > > > > > 
> > > > > > > I might be missing something, but is the phi actually needed or would it be possible to compute the active lane mask at the beginning of each iteration instead of at the end of the iteration?
> > > > > > Hi @fhahn, perhaps I've misunderstood your question here, but actually the point of this patch is to do the exact opposite of that, i.e. we *don't* want to generate the active lane mask for the current iteration - we want to create the mask for the next iteration. This requires a PHI to carry the live value around the loop and is the only way to use the mask for control flow because at the point of branching we want to know if there are any active lanes in the mask for the *next* iteration.
> > > > > > 
> > > > > > With particular reference to SVE, the motivation for this work is to use the 'whilelo' instruction to both generate the lane mask and set the flags to branch on. Effectively, the whilelo instruction is doing the comparison already, which makes the traditional scalar IV comparison redundant.
> > > > > > 
> > > > > > I could be wrong, but I believe this form of vectorised loop would be beneficial for some other targets with a predicated instruction set too, such as RISC-V.
> > > > > > With particular reference to SVE, the motivation for this work is to use the 'whilelo' instruction to both generate the lane mask and set the flags to branch on. 
> > > > > 
> > > > > Oh right, I missed this in the test case I was looking at! I originally thought the intention of the new phi recipe was to encode some extra guarantees/information like we do for inductions or reductions. 
> > > > > 
> > > > > From the latest comment, it sounds like there would be no need to have a new recipe class for the phi, but perhaps VPWidenPHIRecipe could be used instead, if the setup can be moved to the pre-header.
> > > > So I actually tried doing exactly this initially, but I think that VPWidenPHIRecipe requires an underlying scalar instruction to widen due to the execute function that lives in LoopVectorize.cpp:
> > > > 
> > > >   void VPWidenPHIRecipe::execute(VPTransformState &State) {
> > > >     State.ILV->widenPHIInstruction(cast<PHINode>(getUnderlyingValue()), this,
> > > >                                    State);
> > > >   }
> > > > 
> > > > but the PHI node does not exist in the original scalar loop. It's not currently possible to widen a PHI that didn't previously exist, which means I would have to modify VPWidenPHIRecipe to test for the existence of an underlying value and take different paths accordingly.
> > > > So I actually tried doing exactly this initially, but I think that VPWidenPHIRecipe requires an underlying scalar instruction to widen due to the execute function that lives in LoopVectorize.cpp:
> > > 
> > > 
> > > Yeah this was a bit unfortunate! I landed 2 patches that removed the unnecessary dependence on the underlying instruction by instead using the type of its operand.
> > OK well thanks a lot for tidying that class up! The only problem is that even after your patches landed VPWidenPhiRecipe::execute only ever deals with Part 0, whereas we need a Phi for every Part. So unfortunately I still can't use the class in it's current state. If you have suggestions about how to fix this I'm happy to take another look? Perhaps I can add a boolean flag to the VPWidenPhiRecipe constructor to indicate how many parts to generate?
> Hm, let me double check!
Ok it looks like the only reason is that VPWidenPHIRecipe is only used in plans for outer-loop vectorization and there interleaving isn't supported.

So I think it should be fine to just extend it to generate phis for all parts. It would be beneficial to ensure we use generic recipes where possible to avoid duplication and confusion. This will also help with further unifying the split between native/inner loop planning. 


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1551
+        PHINode::Create(StartMask->getType(), 2, "active.lane.mask",
+                        &*State.CFG.PrevBB->getFirstInsertionPt());
+    EntryPart->addIncoming(StartMask, VectorPH);
----------------
Can `Builder.CreatePhi` be used without the need to manually pick the insertion point? 


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:773
+  /// An optional name for the instruction.
+  std::string Name;
+
----------------
This should be documented more clearly. At the moment this is used as the name of the generated IR instruction? Also, this is only used for `VPInstruction::ActiveLaneMask` which is a bit surprising.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2573
+
+  VPActiveLaneMaskPHIRecipe *getActiveLaneMaskPhi() const { return LaneMask; }
+
----------------
Do we need to keep this as member variable? I think at the moment we generally avoid having links to recipes here, as this requires transforms that may remove or replace recipes to be aware of this state (see the similar `getCanonicalIV`)

It should be trivial to find the active lane mask phi recipe, if there is one (just look at the phi recipes of the header)


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

https://reviews.llvm.org/D125301