[PATCH] D158779: [VPlan] Add active-lane-mask as VPlan-to-VPlan transformation.
Ayal Zaks via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Sep 19 11:04:12 PDT 2023
Ayal added inline comments.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8951
+ bool HasNUW = true;
+ addCanonicalIVRecipes(*Plan, Legal->getWidestInductionType(), true,
+ DebugLoc());
----------------
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:869
+ auto *CanonicalIVIncrement =
+ cast<VPInstruction>(CanonicalIVPHI->getBackedgeValue());
+ CanonicalIVIncrement->dropPoisonGeneratingFlags();
----------------
nit: worth commenting why poison generating flags are dropped from CanonicalIVIncrement.
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:956
+ Plan, DataAndControlFlowWithoutRuntimeCheck)
+ : nullptr;
+
----------------
Instead of setting to nullptr, create an ALM instruction following canonical IV widened/incrementedForPart placing both before first non-phi of loop header, to make sure the former can serve all potential users of ICMP_ULE's?
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:961
+ // active-lane-mask.
+ VPValue *BTC = Plan.getOrCreateBackedgeTakenCount();
+ vp_for_all_users(BTC, [&](VPUser *U) {
----------------
Only immediate users of BTC are sought, suffice to traverse them via
` for (VPUser &U : BTC->users()) {`
rather than
` vp_for_all_users() {` ?
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:968
+ !isa<VPWidenCanonicalIVRecipe>(CompareToReplace->getOperand(0)) ||
+ CompareToReplace->getOperand(1) != BTC) {
+ return;
----------------
nit: can assert operand 1 is BTC, given that operand 0 is not and the 2-operand compare uses BTC.
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:976
+ LaneMask = new VPInstruction(VPInstruction::ActiveLaneMask,
+ {WideCanonicalIV, Plan.getTripCount()},
+ nullptr, "active.lane.mask");
----------------
The first operand of ALM when addVPLaneMaskPhiAndUpdateExitBranch() generates it in
- preheader is a `CanonicalIVIncrementForPart(StartV)`
- latch is `CanonicalIVIncrementForPart(CanonicalIVPHI/CanonicalIVIncrement)`
When generating it here in header it is `VPWidenCanonicalIVRecipe(CanonicalIVPHI)`.
Can ALM use CanonicalIVIncrementForPart(CanonicalIVPHI) here too, for consistency?
Are VPWidenCanonicalIVRecipe and CanonicalIVIncrementForPart interchangeable, are both needed?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D158779/new/
https://reviews.llvm.org/D158779
More information about the llvm-commits
mailing list