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

Paul Walker via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jun 30 05:58:15 PDT 2022


paulwalker-arm accepted this revision.
paulwalker-arm added a comment.
This revision is now accepted and ready to land.

A few comment related issues but otherwise looks good to me.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8662
 
+  // Add a CanonicalIVIncrement{NUW} VPInstruction to increment it the scalar
+  // IV by VF * UF.
----------------
remove the "it"?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8697-8698
+    // to the exit block.
+    auto *NotMask = new VPInstruction(VPInstruction::Not, ALM, DL);
+    EB->appendRecipe(NotMask);
+
----------------
david-arm wrote:
> paulwalker-arm wrote:
> > This seems a bit wasteful given it's only the first lane we care about.  perhaps `VPInstruction::BranchOnCount` could take an `invert` flag that switches the branch destinations?
> It's not as trivial as you'd think passing in flags to VPInstructions. :( You have to create an IR Constant, pass it in as a 3rd VPValue argument to a VPInstruction and mark it as IR "live value in" operand. We do end up with nice code after InstCombine, since it swaps the labels around for us anyway.
> 
> If you really prefer me to go down this route I can - it's just a little awkward that's all.
No need if this is how the interface works.  Perhaps VPIntruction could do with some domain specific flags, which might also simplify the NUW split. Either way it's not directly relevant to this patch.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:796-797
+
+    // The loop step is equal to the vectorization factor (num of SIMD
+    // elements) times the unroll factor (num of SIMD instructions).
+    Value *Step = createStepForVF(Builder, IV->getType(), State.VF, Part);
----------------
This looks to be copied from `case VPInstruction::CanonicalIVIncrement` but it not really correct within the context of this opcode.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2493
 
+  VPActiveLaneMaskPHIRecipe *LaneMask = nullptr;
+
----------------
Could do with a comment.


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

https://reviews.llvm.org/D125301



More information about the llvm-commits mailing list