[LV] Use wide lane masks as the canonical form when tail-folding & interleaving - #209484
[LV] Use wide lane masks as the canonical form when tail-folding & interleaving#209484kmclaughlin-arm wants to merge 10 commits into
Conversation
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-vectorizers Author: Kerry McLaughlin (kmclaughlin-arm) ChangesPreviously, when passing This PR removes During unrolling, the multiplier is set to UF for ActiveLaneMask and one This does not change the default behaviour for tail-folded loops, which Patch is 265.04 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/209484.diff 16 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index 1929f52ae7d95..98a8f56e7500b 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -255,11 +255,6 @@ static cl::opt<TailFoldingStyle> ForceTailFoldingStyle(
"Use predicated EVL instructions for tail folding. If EVL "
"is unsupported, fallback to data-without-lane-mask.")));
-cl::opt<bool> llvm::EnableWideActiveLaneMask(
- "enable-wide-lane-mask", cl::init(false), cl::Hidden,
- cl::desc("Enable use of wide lane masks when used for control flow in "
- "tail-folded loops"));
-
static cl::opt<bool> EnableInterleavedMemAccesses(
"enable-interleaved-mem-accesses", cl::init(false), cl::Hidden,
cl::desc("Enable vectorization on interleaved memory accesses in a loop"));
@@ -1221,15 +1216,6 @@ class LoopVectorizationCostModel {
return PartialAliasMaskingStatus == AliasMaskingStatus::Enabled;
}
- /// Returns true if the use of wide lane masks is requested and the loop is
- /// using tail-folding with a lane mask for control flow.
- bool useWideActiveLaneMask() const {
- if (!EnableWideActiveLaneMask)
- return false;
-
- return getTailFoldingStyle() == TailFoldingStyle::DataAndControlFlow;
- }
-
/// Returns true if the instructions in this block requires predication
/// for any reason, e.g. because tail folding now requires a predicate
/// or because the block in the original loop was predicated.
@@ -3651,12 +3637,10 @@ LoopVectorizationPlanner::selectInterleaveCount(VPlan &Plan, ElementCount VF,
// 3. We don't interleave if we think that we will spill registers to memory
// due to the increased register pressure.
- // Only interleave tail-folded loops if wide lane masks are requested, as the
- // overhead of multiple instructions to calculate the predicate is likely
- // not beneficial. If an epilogue is not allowed for any other reason,
- // do not interleave.
- if (!CM.isEpilogueAllowed() &&
- !(CM.preferTailFoldedLoop() && CM.useWideActiveLaneMask()))
+ // Do not interleave tail-folded loops, as the overhead of multiple
+ // instructions to calculate the predicate is likely not beneficial.
+ // If an epilogue is not allowed for any other reason, do not interleave.
+ if (!CM.isEpilogueAllowed() || CM.preferTailFoldedLoop())
return 1;
if (any_of(Plan.getVectorLoopRegion()->getEntryBasicBlock()->phis(),
diff --git a/llvm/lib/Transforms/Vectorize/VPlan.h b/llvm/lib/Transforms/Vectorize/VPlan.h
index 47c34157533e4..9433ddb4a38e7 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.h
+++ b/llvm/lib/Transforms/Vectorize/VPlan.h
@@ -1222,15 +1222,18 @@ class LLVM_ABI_FOR_TEST VPInstruction : public VPRecipeWithIRFlags,
public:
/// VPlan opcodes, extending LLVM IR with idiomatics instructions.
enum {
- FirstOrderRecurrenceSplice =
- Instruction::OtherOpsEnd + 1, // Combines the incoming and previous
- // values of a first-order recurrence.
+ FirstOrderRecurrenceSplice = Instruction::OtherOpsEnd +
+ 1, // Combines the incoming and previous
+ // values of a first-order recurrence.
Not,
// Creates a mask where each lane is active (true) whilst the current
// counter (first operand + index) is less than the second operand. i.e.
// mask[i] = icmpt ult (op0 + i), op1
// The size of the mask returned is VF * Multiplier (UF, third op).
ActiveLaneMask,
+ // Extracts each unrolled part of a widened active lane mask when used
+ // for control flow.
+ ExtractSubvectorForPart,
ExplicitVectorLength,
// Represents the incoming loop-invariant alias-mask. All memory accesses
// in the loop must stay within the active lanes.
diff --git a/llvm/lib/Transforms/Vectorize/VPlanPatternMatch.h b/llvm/lib/Transforms/Vectorize/VPlanPatternMatch.h
index 7e7d0f9b6a280..c185d157d23a5 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanPatternMatch.h
+++ b/llvm/lib/Transforms/Vectorize/VPlanPatternMatch.h
@@ -442,6 +442,12 @@ m_ExtractLastLaneOfLastPart(const Op0_t &Op0) {
return m_ExtractLastLane(m_ExtractLastPart(Op0));
}
+template <typename Op0_t, typename Op1_t>
+inline VPInstruction_match<VPInstruction::ExtractSubvectorForPart, Op0_t, Op1_t>
+m_ExtractSubvectorForPart(const Op0_t &Op0, const Op1_t &Op1) {
+ return m_VPInstruction<VPInstruction::ExtractSubvectorForPart>(Op0, Op1);
+}
+
template <typename Op0_t>
inline VPInstruction_match<VPInstruction::ExtractPenultimateElement, Op0_t>
m_ExtractPenultimateElement(const Op0_t &Op0) {
diff --git a/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp b/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
index 3fbeb7e772a2e..e5f67a412bd03 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
@@ -658,6 +658,7 @@ unsigned VPInstruction::getNumOperandsForOpcode() const {
case VPInstruction::WideIVStep:
case VPInstruction::CalculateTripCountMinusVF:
case VPInstruction::ResumeForEpilogue:
+ case VPInstruction::ExtractSubvectorForPart:
return 2;
case Instruction::InsertElement:
case Instruction::Select:
@@ -793,12 +794,12 @@ Value *VPInstruction::generate(VPTransformState &State) {
// If this part of the active lane mask is scalar, generate the CMP directly
// to avoid unnecessary extracts.
- if (State.VF.isScalar())
+ unsigned Multiplier = cast<VPConstantInt>(getOperand(2))->getZExtValue();
+ if (State.VF.isScalar() && Multiplier == 1)
return Builder.CreateCmp(CmpInst::Predicate::ICMP_ULT, VIVElem0, ScalarTC,
Name);
- ElementCount EC = State.VF.multiplyCoefficientBy(
- cast<VPConstantInt>(getOperand(2))->getZExtValue());
+ ElementCount EC = State.VF.multiplyCoefficientBy(Multiplier);
auto *PredTy = VectorType::get(Builder.getInt1Ty(), EC);
return Builder.CreateIntrinsic(Intrinsic::get_active_lane_mask,
{PredTy, ScalarTC->getType()},
@@ -1104,6 +1105,33 @@ Value *VPInstruction::generate(VPTransformState &State) {
return Result;
}
+ case VPInstruction::ExtractSubvectorForPart: {
+ Type *DstTy = VectorType::get(getScalarType(), State.VF);
+ unsigned Part = cast<VPConstantInt>(getOperand(1))->getZExtValue();
+
+ // Return Src if it's an ActiveLaneMask with a multiplier of 1.
+ Value *Src = State.get(getOperand(0));
+ if (Part == 0 && match(getOperand(0),
+ m_ActiveLaneMask(m_VPValue(), m_VPValue(), m_One())))
+ return Src;
+
+ // If the VF is scalar & this is an extract of an active lane mask,
+ // generate an ICMP directly.
+ VPValue *Start, *TC;
+ if (State.VF.isScalar() &&
+ match(getOperand(0),
+ m_ActiveLaneMask(m_VPValue(Start), m_VPValue(TC), m_VPValue()))) {
+ Value *StartV = State.get(Start);
+ if (Part > 0)
+ StartV = Builder.CreateAdd(State.get(Start), State.get(getOperand(1)));
+ return Builder.CreateCmp(CmpInst::Predicate::ICMP_ULT, StartV,
+ State.get(TC));
+ }
+
+ auto *Idx = ConstantInt::get(Builder.getInt64Ty(),
+ State.VF.getKnownMinValue() * Part);
+ return Builder.CreateExtractVector(DstTy, Src, Idx);
+ }
default:
llvm_unreachable("Unsupported opcode for instruction");
}
@@ -1628,6 +1656,7 @@ bool VPInstruction::opcodeMayReadOrWriteFromMemory() const {
case VPInstruction::FirstActiveLane:
case VPInstruction::LastActiveLane:
case VPInstruction::ExtractLastActive:
+ case VPInstruction::ExtractSubvectorForPart:
case VPInstruction::FirstOrderRecurrenceSplice:
case VPInstruction::LogicalAnd:
case VPInstruction::LogicalOr:
@@ -1802,6 +1831,9 @@ void VPInstruction::printRecipe(raw_ostream &O, const Twine &Indent,
case VPInstruction::ExtractPenultimateElement:
O << "extract-penultimate-element";
break;
+ case VPInstruction::ExtractSubvectorForPart:
+ O << "extract-subvector-for-part";
+ break;
case VPInstruction::ComputeReductionResult:
O << "compute-reduction-result";
break;
diff --git a/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp b/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
index 4da74c5146c52..03e8189692d5c 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
@@ -2286,105 +2286,6 @@ static bool isConditionTrueViaVFAndUF(VPValue *Cond, VPlan &Plan,
return SE.isKnownPredicate(CmpInst::ICMP_EQ, VectorTripCount, C);
}
-/// Try to replace multiple active lane masks used for control flow with
-/// a single, wide active lane mask instruction followed by multiple
-/// extract subvector intrinsics. This applies to the active lane mask
-/// instructions both in the loop and in the preheader.
-/// Incoming values of all ActiveLaneMaskPHIs are updated to use the
-/// new extracts from the first active lane mask, which has it's last
-/// operand (multiplier) set to UF.
-static bool tryToReplaceALMWithWideALM(VPlan &Plan, ElementCount VF,
- unsigned UF) {
- if (!EnableWideActiveLaneMask || !VF.isVector() || UF == 1)
- return false;
-
- VPRegionBlock *VectorRegion = Plan.getVectorLoopRegion();
- VPBasicBlock *ExitingVPBB = VectorRegion->getExitingBasicBlock();
- auto *Term = &ExitingVPBB->back();
-
- using namespace llvm::VPlanPatternMatch;
- if (!match(Term, m_BranchOnCond(m_Not(m_ActiveLaneMask(
- m_VPValue(), m_VPValue(), m_VPValue())))))
- return false;
-
- auto *Header = cast<VPBasicBlock>(VectorRegion->getEntry());
- LLVMContext &Ctx = Plan.getContext();
-
- auto ExtractFromALM = [&](VPInstruction *ALM,
- SmallVectorImpl<VPValue *> &Extracts) {
- DebugLoc DL = ALM->getDebugLoc();
- for (unsigned Part = 0; Part < UF; ++Part) {
- SmallVector<VPValue *> Ops;
- Ops.append({ALM, Plan.getConstantInt(64, VF.getKnownMinValue() * Part)});
- auto *Ext =
- new VPWidenIntrinsicRecipe(Intrinsic::vector_extract, Ops,
- IntegerType::getInt1Ty(Ctx), {}, {}, DL);
- Extracts[Part] = Ext;
- Ext->insertAfter(ALM);
- }
- };
-
- // Create a list of each active lane mask phi, ordered by unroll part.
- SmallVector<VPActiveLaneMaskPHIRecipe *> Phis(UF, nullptr);
- for (VPRecipeBase &R : Header->phis()) {
- auto *Phi = dyn_cast<VPActiveLaneMaskPHIRecipe>(&R);
- if (!Phi)
- continue;
- VPValue *Index = nullptr;
- match(Phi->getBackedgeValue(),
- m_ActiveLaneMask(m_VPValue(Index), m_VPValue(), m_VPValue()));
- assert(Index && "Expected index from ActiveLaneMask instruction");
-
- uint64_t Part;
- if (match(Index,
- m_VPInstruction<VPInstruction::CanonicalIVIncrementForPart>(
- m_VPValue(), m_Mul(m_VPValue(), m_ConstantInt(Part)))))
- Phis[Part] = Phi;
- else {
- // Anything other than a CanonicalIVIncrementForPart is part 0
- assert(!match(
- Index,
- m_VPInstruction<VPInstruction::CanonicalIVIncrementForPart>()));
- Phis[0] = Phi;
- }
- }
-
- assert(all_of(Phis, not_equal_to(nullptr)) &&
- "Expected one VPActiveLaneMaskPHIRecipe for each unroll part");
-
- auto *EntryALM = cast<VPInstruction>(Phis[0]->getStartValue());
- auto *LoopALM = cast<VPInstruction>(Phis[0]->getBackedgeValue());
-
- assert((EntryALM->getOpcode() == VPInstruction::ActiveLaneMask &&
- LoopALM->getOpcode() == VPInstruction::ActiveLaneMask) &&
- "Expected incoming values of Phi to be ActiveLaneMasks");
-
- // When using wide lane masks, the return type of the get.active.lane.mask
- // intrinsic is VF x UF (last operand).
- VPValue *ALMMultiplier = Plan.getConstantInt(64, UF);
- EntryALM->setOperand(2, ALMMultiplier);
- LoopALM->setOperand(2, ALMMultiplier);
-
- // Create UF x extract vectors and insert into preheader.
- SmallVector<VPValue *> EntryExtracts(UF);
- ExtractFromALM(EntryALM, EntryExtracts);
-
- // Create UF x extract vectors and insert before the loop compare & branch,
- // updating the compare to use the first extract.
- SmallVector<VPValue *> LoopExtracts(UF);
- ExtractFromALM(LoopALM, LoopExtracts);
- VPInstruction *Not = cast<VPInstruction>(Term->getOperand(0));
- Not->setOperand(0, LoopExtracts[0]);
-
- // Update the incoming values of active lane mask phis.
- for (unsigned Part = 0; Part < UF; ++Part) {
- Phis[Part]->setStartValue(EntryExtracts[Part]);
- Phis[Part]->setBackedgeValue(LoopExtracts[Part]);
- }
-
- return true;
-}
-
/// Try to simplify the branch condition of \p Plan. This may restrict the
/// resulting plan to \p BestVF and \p BestUF.
static bool simplifyBranchConditionForVFAndUF(VPlan &Plan, ElementCount BestVF,
@@ -2401,9 +2302,13 @@ static bool simplifyBranchConditionForVFAndUF(VPlan &Plan, ElementCount BestVF,
m_CombineOr(m_CanIVInc, m_c_Add(m_CanIVInc, m_LiveIn())),
m_VPValue())) ||
match(Term, m_BranchOnCond(m_Not(m_ActiveLaneMask(
- m_VPValue(), m_VPValue(), m_VPValue()))))) {
+ m_VPValue(), m_VPValue(), m_VPValue())))) ||
+ match(Term, m_BranchOnCond(m_Not(m_ExtractSubvectorForPart(
+ m_ActiveLaneMask(m_VPValue(), m_VPValue(), m_VPValue()),
+ m_ZeroInt()))))) {
// Try to simplify the branch condition if VectorTC <= VF * UF when the
- // latch terminator is BranchOnCount or BranchOnCond(Not(ActiveLaneMask)).
+ // latch terminator is BranchOnCount, BranchOnCond(Not(ActiveLaneMask))
+ // or BranchOnCond(Not(ExtractSubvectorForPart(ActiveLaneMask), 0))
const SCEV *VectorTripCount =
vputils::getSCEVExprForVPValue(&Plan.getVectorTripCount(), PSE);
if (isa<SCEVCouldNotCompute>(VectorTripCount))
@@ -2482,8 +2387,8 @@ void VPlanTransforms::optimizeForVFAndUF(VPlan &Plan, ElementCount BestVF,
assert(Plan.hasVF(BestVF) && "BestVF is not available in Plan");
assert(Plan.hasUF(BestUF) && "BestUF is not available in Plan");
- bool MadeChange = tryToReplaceALMWithWideALM(Plan, BestVF, BestUF);
- MadeChange |= simplifyBranchConditionForVFAndUF(Plan, BestVF, BestUF, PSE);
+ bool MadeChange =
+ simplifyBranchConditionForVFAndUF(Plan, BestVF, BestUF, PSE);
MadeChange |= optimizeVectorInductionWidthForTCAndVFUF(Plan, BestVF, BestUF);
if (MadeChange) {
@@ -2995,6 +2900,9 @@ addVPLaneMaskPhiAndUpdateExitBranch(VPlan &Plan) {
auto *EntryALM = Builder.createNaryOp(VPInstruction::ActiveLaneMask,
{EntryIncrement, TC, ALMMultiplier}, DL,
"active.lane.mask.entry");
+ EntryALM = Builder.createNaryOp(VPInstruction::ExtractSubvectorForPart,
+ {EntryALM, Plan.getConstantInt(64, 0)}, DL,
+ "extract.entry.alm.part");
// Now create the ActiveLaneMaskPhi recipe in the main loop using the
// preheader ActiveLaneMask instruction.
@@ -3013,6 +2921,9 @@ addVPLaneMaskPhiAndUpdateExitBranch(VPlan &Plan) {
auto *ALM = Builder.createNaryOp(VPInstruction::ActiveLaneMask,
{InLoopIncrement, TC, ALMMultiplier}, DL,
"active.lane.mask.next");
+ ALM = Builder.createNaryOp(VPInstruction::ExtractSubvectorForPart,
+ {ALM, Plan.getConstantInt(64, 0)}, DL,
+ "extract.next.alm.part");
LaneMaskPhi->addBackedgeValue(ALM);
// Replace the original terminator with BranchOnCond. We have to invert the
diff --git a/llvm/lib/Transforms/Vectorize/VPlanTransforms.h b/llvm/lib/Transforms/Vectorize/VPlanTransforms.h
index 85375625d34b5..045dd551c2027 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanTransforms.h
+++ b/llvm/lib/Transforms/Vectorize/VPlanTransforms.h
@@ -39,7 +39,6 @@ class VPRecipeBuilder;
struct VFRange;
LLVM_ABI_FOR_TEST extern cl::opt<bool> VerifyEachVPlan;
-LLVM_ABI_FOR_TEST extern cl::opt<bool> EnableWideActiveLaneMask;
#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
LLVM_ABI_FOR_TEST extern cl::opt<bool> VPlanPrintBeforeAll;
diff --git a/llvm/lib/Transforms/Vectorize/VPlanUnroll.cpp b/llvm/lib/Transforms/Vectorize/VPlanUnroll.cpp
index 594c201d5a8cb..832b88c2c0252 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanUnroll.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanUnroll.cpp
@@ -473,6 +473,31 @@ void UnrollState::unrollBlock(VPBlockBase *VPB) {
continue;
}
+ if (match(&R,
+ m_ActiveLaneMask(m_VPValue(), m_VPValue(Op1), m_VPValue(Op2)))) {
+ auto *ALM = cast<VPInstruction>(&R);
+ auto *User = ALM->getSingleUser();
+ // Widen ActiveLaneMask when used for control flow.
+ if (User && match(User, m_ExtractSubvectorForPart(m_Specific(ALM),
+ m_ZeroInt()))) {
+ addUniformForAllParts(ALM);
+ ALM->setOperand(2, Plan.getConstantInt(64, UF));
+ continue;
+ }
+ }
+
+ if (match(&R, m_ExtractSubvectorForPart(m_VPValue(Op0), m_VPValue(Op1)))) {
+ VPRecipeBase *InsertPt = &R;
+ for (unsigned Part = 1; Part != UF; ++Part) {
+ auto *Ext = new VPInstruction(VPInstruction::ExtractSubvectorForPart,
+ {Op0, Plan.getConstantInt(64, Part)});
+ Ext->insertAfter(InsertPt);
+ addRecipeForPart(&R, Ext, Part);
+ InsertPt = Ext;
+ }
+ continue;
+ }
+
auto *SingleDef = dyn_cast<VPSingleDefRecipe>(&R);
if (SingleDef && vputils::isUniformAcrossVFsAndUFs(SingleDef)) {
addUniformForAllParts(SingleDef);
diff --git a/llvm/test/Transforms/LoopVectorize/AArch64/fixed-wide-lane-mask.ll b/llvm/test/Transforms/LoopVectorize/AArch64/fixed-wide-lane-mask.ll
index 17cc76c52c7e3..bff2a61711124 100644
--- a/llvm/test/Transforms/LoopVectorize/AArch64/fixed-wide-lane-mask.ll
+++ b/llvm/test/Transforms/LoopVectorize/AArch64/fixed-wide-lane-mask.ll
@@ -1,7 +1,7 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --filter-out-after "^middle.block:" --version 4
-; RUN: opt -S -passes=loop-vectorize -scalable-vectorization=off -tail-folding-policy=must-fold-tail -enable-wide-lane-mask \
+; RUN: opt -S -passes=loop-vectorize -scalable-vectorization=off -tail-folding-policy=must-fold-tail \
; RUN: -force-vector-width=4 -force-vector-interleave=1 < %s | FileCheck %s -check-prefix CHECK-UF1
-; RUN: opt -S --passes=loop-vectorize -scalable-vectorization=off -tail-folding-policy=must-fold-tail -enable-wide-lane-mask \
+; RUN: opt -S --passes=loop-vectorize -scalable-vectorization=off -tail-folding-policy=must-fold-tail \
; RUN: -force-vector-width=4 -force-vector-interleave=4 < %s | FileCheck %s -check-prefix CHECK-UF4
target triple = "aarch64-unknown-linux"
@@ -10,13 +10,13 @@ define void @fixed_wide_active_lane_mask(ptr noalias %dst, ptr noalias readonly
; CHECK-UF1-LABEL: define void @fixed_wide_active_lane_mask(
; CHECK-UF1-SAME: ptr noalias [[DST:%.*]], ptr noalias readonly [[SRC:%.*]], i64 [[N:%.*]]) #[[ATTR0:[0-9]+]] {
; CHECK-UF1-NEXT: entry:
-; CHECK-UF1-NEXT: br label [[ENTRY:%.*]]
+; CHECK-UF1-NEXT: br label [[VECTOR_PH:%.*]]
; CHECK-UF1: vector.ph:
; CHECK-UF1-NEXT: [[ACTIVE_LANE_MASK_ENTRY:%.*]] = call <4 x i1> @llvm.get.active.lane.mask.v4i1.i64(i64 0, i64 [[N]])
; CHECK-UF1-NEXT: br label [[VECTOR_BODY:%.*]]
; CHECK-UF1: vector.body:
-; CHECK-UF1-NEXT: [[INDEX:%.*]] = phi i64 [ 0, [[ENTRY]] ], [ [[INDEX_NEXT:%.*]], [[VECTOR_BODY]] ]
-; CHECK-UF1-NEXT: [[ACTIVE_LANE_MASK:%.*]] = phi <4 x i1> [ [[ACTIVE_LANE_MASK_ENTRY]], [[ENTRY]] ], [ [[ACTIVE_LANE_MASK_NEXT:%.*]], [[VECTOR_BODY]] ]
+; CHECK-UF1-NEXT: [[INDEX:%.*]] = phi i64 [ 0, [[VECTOR_PH]] ], [ [[INDEX_NEXT:%.*]], [[VECTOR_BODY]] ]
+; CHECK-UF1-NEXT: [[ACTIVE_LANE_MASK:%.*]] = phi <4 x i1> [ [[ACTIVE_LANE_MASK_ENTRY]], [[VECTOR_PH]] ], [ [[ACTIVE_LANE_MASK_NEXT:%.*]], [[VECTOR_BODY]] ]
; CHECK-UF1-NEXT: [[TMP3:%.*]] = load i32, ptr [[SRC]], align 4
; CHECK-UF1-NEXT: [[BROADCAST_SPLATINSERT:%.*]] = insertelement...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
fa3a96d to
7fa724f
Compare
🪟 Windows x64 Test Results
✅ The build succeeded and all tests passed. |
7fa724f to
4d72990
Compare
| if (User && match(User, m_ExtractSubvectorForPart(m_Specific(ALM), | ||
| m_ZeroInt()))) { |
There was a problem hiding this comment.
This seems fragile. Is the rational for ExtractSubvectorForPart to help identify this use case? If so I'm wondering if it's better to create ActiveLaneMaskForControlFlow instead?
Not sure if the argument is sound but I'm thinking this will hide the subvector stuff until after unrolling where it matters? Essentially the only difference between ActiveLaneMask and ActiveLaneMaskForControlFlow is how it is unrolled.
If this is valid then perhaps ActiveLaneMaskForControlFlow is too specific a name? It could be something like WideActiveLaneMask or UnrolledActiveLaneMask to match the intend. The key bit is that you're canonicalising to always use this for control flow rather than ActiveLaneMask.
There was a problem hiding this comment.
ExtractSubvectorForPart is my ask as it's useful for other forms of masking and widening outside of this case (specifically predicate-as-counter and representing multi-vector loads/stores).
Separating the widening of the mask from the sub vector extraction allows the same VPlan operations to be reused to represent both styles of loop (with a relatively simple VPlan transform)... Though my patches demonstrating this are currently downstream.
There was a problem hiding this comment.
If ExtractSubvectorForPart has value separately then that's great, but it should be added for those use cases rather than potentially being wedged in here for convenience.
What I don't like about the current approach is that you need to construct a very specific VPInstruction sequence, that latter has to be identified, so the control-flow mask is constructed correctly. That feels wrong to me as I believe we should always be able to handle VPInstructions in isolation.
That is why I see two different active.lane.mask operations. One that represents a single interleave factor and is unrolled as you'd expect, and a second that represents the entire vector loop that is not duplicated when unrolled but instead widened with each part extracting their section from it. Doing this means when either is interrogated the semantics of the operation remain clear.
There was a problem hiding this comment.
That is why I see two different active.lane.mask operations. One that represents a single interleave factor and is unrolled as you'd expect, and a second that represents the entire vector loop that is not duplicated when unrolled but instead widened with each part extracting their section from it. Doing this means when either is interrogated the semantics of the operation remain clear.
The intention was this approach (separating widening the mask from the extraction), was it allows us to reuse the active.lane.mask widening logic for the future predicate-as-counter work. With your suggestion we'd need to add a third variation of the "ActiveLaneMask" operation, with different unrolling semantics.
There was a problem hiding this comment.
It's possible I've misread you're previous comments, but if we could keep the current IR structure but simply have WideActiveLaneMask + ExtractSubvectorForPart, where unrolling WideActiveLaneMask does not insert the extracts that'd also work fine for my use case. That'd remove the need to match the users of the lane mask during unrolling.
There was a problem hiding this comment.
I can only review what I see, which at the moment looks fragile because the correct interpretation of a VPInstruction requires you to look at its users. If your use case introduces a third interpretation then perhaps that suggest another operation, or the active-lane-mask instruction requires more internal state to identify the specific operation. It's hard without knowing the details and personally I think this PR should stand on its own. If your use case then requires alterations then so be it.
There was a problem hiding this comment.
Right, my suggestion above is to have ActiveLaneMask (used for masking without control flow) and WideActiveLaneMask/ActiveLaneMaskForControlFlow used for masking with control flow. The behavior of WideActiveLaneMask would be always to widen the mask. The sub parts of the mask would be extracted with ExtractSubvectorForPart.
This keeps the basic operations simple (and directly models the extracts). WideActiveLaneMask simply widens the mask, ExtractSubvectorForPart only extracts the subvectors. Both operations can be unrolled without looking at the users, and are generic enough to be reused without refactoring again.
There was a problem hiding this comment.
Works for me. Thanks.
There was a problem hiding this comment.
I've added a new VPInstruction called ActiveLaneMaskForControlFlow. I'm happy to change the name to something like WideActiveLaneMask if that makes more sense, but I think this better reflects the tail-folding style of loops that this will be created for.
This removes the dependency on a specific instruction sequence, but will hopefully still be useful for @MacDue's predicate-as-counter work.
There was a problem hiding this comment.
Thanks @kmclaughlin-arm. If there's a vote, I prefer WideActiveLaneMask because it better articulates the difference verses ActiveLaneMask (i.e. one gets widened rather than duplicated when unrolled). Plus I don't see why we'd block non-control use cases if they ever present themselves.
MacDue
left a comment
MacDue
left a comment
There was a problem hiding this comment.
This generally LGTM, just a few small comments. I'll defer to Paul for the naming.
These VPInstructions would also be useful for my later work too, thanks 👍
| // Extracts each unrolled part of a widened active lane mask when used | ||
| // for control flow. |
There was a problem hiding this comment.
nit: Maybe phrase this more generally.
| // Extracts each unrolled part of a widened active lane mask when used | |
| // for control flow. | |
| // Extracts each unrolled part of a (VFxUF) widened vector/mask. |
There was a problem hiding this comment.
While we're talking about naming, can this be ExtractVectorForPart :)
There was a problem hiding this comment.
Renamed new VPInstrutions to WideActiveLaneMask & ExtractVectorForPart
4a9bea5 to
4223140
Compare
| // mask[i] = icmpt ult (op0 + i), op1 | ||
| // The size of the mask returned is VF * Multiplier (UF, third op). | ||
| ActiveLaneMask, | ||
| ActiveLaneMaskForControlFlow, |
There was a problem hiding this comment.
please also document this one,
| // not beneficial. If an epilogue is not allowed for any other reason, | ||
| // do not interleave. | ||
| if (!CM.isEpilogueAllowed() && | ||
| !(CM.preferTailFoldedLoop() && CM.useWideActiveLaneMask())) |
There was a problem hiding this comment.
does this change behavior for other tail folding styles that do not use ALM?
There was a problem hiding this comment.
Is this just reverting back to the original code before useWideActiveLaneMask was introduced?
There was a problem hiding this comment.
This was intended to revert selectInterleaveCount back to the original code before #163387, however CM.preferTailFoldedLoop() should also have been removed. I've removed this in the latest commit, which prevents any tail-folded loop from being interleaved without forcing as it was originally.
| // mask[i] = icmpt ult (op0 + i), op1 | ||
| // The size of the mask returned is VF * Multiplier (UF, third op). | ||
| ActiveLaneMask, | ||
| WideActiveLaneMask, |
There was a problem hiding this comment.
please document to make clear what the difference to regular ActiveeLaneMask is
There was a problem hiding this comment.
Added a new comment for WideActiveLaneMask which describes the unrolling behaviour (also updated the ActiveLaneMask description to remove Multiplier).
| m_VPValue()))) { | ||
| Value *StartV = State.get(Start); | ||
| if (Part > 0) | ||
| StartV = Builder.CreateAdd(StartV, State.get(getOperand(1))); |
There was a problem hiding this comment.
I think StartV may have a different type than operand 0. If so, could you add a test. Also, we already looking up operand 1 above, better assign to variable to clarify
There was a problem hiding this comment.
Thanks @fhahn, this was not correct when StartV was a different type from Op1. I've changed this to create the add using the same type as StartV and added a new test in wide-alm-scalar-vf.ll.
| State.get(TC)); | ||
| } | ||
|
|
||
| auto *Idx = ConstantInt::get(Builder.getInt64Ty(), |
There was a problem hiding this comment.
I think there's a Builder.getInt64 or something like that
| match(Mask, m_VPInstruction<VPInstruction::ActiveLaneMask>()) || | ||
| match(Mask, m_VPInstruction<VPInstruction::WideActiveLaneMask>())) | ||
| continue; | ||
|
|
||
| if (match(Mask, m_ActiveLaneMask(m_VPValue(), m_VPValue(), m_VPValue()))) | ||
| if (match(Mask, m_ActiveLaneMask(m_VPValue(), m_VPValue(), m_VPValue())) || | ||
| match(Mask, | ||
| m_WideActiveLaneMask(m_VPValue(), m_VPValue(), m_VPValue()))) |
There was a problem hiding this comment.
I think the matches above are enough, I've removed these.
| // We can't use StartV directly in the WideActiveLaneMask | ||
| // VPInstruction, since we have to take unrolling into account. | ||
| // Each part needs to start at Part * VF |
There was a problem hiding this comment.
Is this comment still relevant? For WideActiveLaneMask we widen when unrolling, removing the need for Part * VF?
There was a problem hiding this comment.
I think EntryIncrement is still needed to make sure ExtractVectorForPart is unrolled correctly, but I've reworded the comment and moved it further down to where the instruction is created.
| // Create the WideActiveLaneMask instruction using the correct | ||
| // start values. |
There was a problem hiding this comment.
I know this is an existing comment, but do we really need to specifically say to use the "correct values", I would expect that to be the norm?
There was a problem hiding this comment.
Removed this comment, since I think it's clear from the names what the values used to create the mask are.
| ; CHECK-UF1-SAME: ptr noalias [[DST:%.*]], ptr readonly [[SRC:%.*]], i64 [[N:%.*]]) #[[ATTR0:[0-9]+]] { | ||
| ; CHECK-UF1-NEXT: entry: | ||
| ; CHECK-UF1-NEXT: br label [[VECTOR_PH1:%.*]] | ||
| ; CHECK-UF1-NEXT: br label [[VECTOR_PH:%.*]] |
There was a problem hiding this comment.
Perhaps manually regenerate these CHECK lines in a pre-commit to reduce the diff here?
| ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --check-globals none --filter-out-after "^middle.block:" --version 4 | ||
| ; RUN: opt -S --passes=loop-vectorize -tail-folding-policy=must-fold-tail -enable-wide-lane-mask -force-vector-interleave=1 < %s | FileCheck %s -check-prefix CHECK-UF1 | ||
| ; RUN: opt -S --passes=loop-vectorize -tail-folding-policy=must-fold-tail -enable-wide-lane-mask -force-vector-interleave=4 < %s | FileCheck %s -check-prefix CHECK-UF4 | ||
| ; RUN: opt -S --passes=loop-vectorize -enable-wide-lane-mask -tail-folding-policy=must-fold-tail < %s | FileCheck %s -check-prefix CHECK-TF |
There was a problem hiding this comment.
Have you removed this RUN line because you don't think it's worth testing that the default interleave count is 1?
There was a problem hiding this comment.
I added this RUN line when I changed selectInterleaveCount to consider interleaving when tail-folding if wide lane masks were requested. I removed it in this PR because we are no longer allowing interleaving of tail-folded loops unless forced, which was the behaviour before I added -enable-wide-lane-mask.
I can add this back in if you think it's still worth having a test to make sure we don't interleave?
| // not beneficial. If an epilogue is not allowed for any other reason, | ||
| // do not interleave. | ||
| if (!CM.isEpilogueAllowed() && | ||
| !(CM.preferTailFoldedLoop() && CM.useWideActiveLaneMask())) |
There was a problem hiding this comment.
Is this just reverting back to the original code before useWideActiveLaneMask was introduced?
| VPValue *ALMMultiplier = | ||
| Plan.getConstantInt(TopRegion->getCanonicalIVType(), 1); | ||
| auto *EntryALM = Builder.createNaryOp(VPInstruction::ActiveLaneMask, | ||
| auto *EntryALM = Builder.createNaryOp(VPInstruction::WideActiveLaneMask, |
There was a problem hiding this comment.
If we're always doing this for all targets regardless of the interleave count do we still need VPInstruction::ActiveLaneMask? Also, I think there is currently work being done by @artagnon to enable simplification of active lane masks with constant operands in the InstSimplifyFolder when detecting these recipes. That's probably fine, but any simplifications will need to now look at both recipes.
There was a problem hiding this comment.
ActiveLaneMask is needed as it will still be used for other styles of tail-folding (where the mask is not used for control-flow).
I'm happy to take a look at InstSimplify after this; I didn't know that there was work being done for ActiveLaneMask, but it sounds like it could apply to WideActiveLaneMask too.
| auto *EntryALM = Builder.createNaryOp(VPInstruction::WideActiveLaneMask, | ||
| {EntryIncrement, TC, ALMMultiplier}, DL, | ||
| "active.lane.mask.entry"); | ||
| EntryALM = Builder.createNaryOp(VPInstruction::ExtractVectorForPart, |
There was a problem hiding this comment.
It might be worth having at least one wide active lane mask test where the inputs are constants, i.e. a loop with a trip count of 9, VF=4, IC=2. I'd expect in future that the two entry parts will be simplified to splats of (i1 1).
There was a problem hiding this comment.
I've added a test for this in wide-alm-const.ll
| auto *ALM = Builder.createNaryOp(VPInstruction::WideActiveLaneMask, | ||
| {InLoopIncrement, TC, ALMMultiplier}, DL, | ||
| "active.lane.mask.next"); | ||
| ALM = Builder.createNaryOp(VPInstruction::ExtractVectorForPart, |
There was a problem hiding this comment.
In theory if we're extracting part 0 of WideActiveLaneMask{0, 9, 1} where VF=4, IC=2, then there's nothing stopping vplan from simplifying this to ActiveLaneMask{0, 9} in future. Is it a requirement for this form to persist until vplan execution? If so, it might be a bit fragile and perhaps worth adding a comment here saying this code should not be simplified. Either that or ensure that we can still use the normal ActiveLaneMask for control flow.
There was a problem hiding this comment.
There's no requirement for this to persist in vplan, although later optimisations which are looking for the pattern of a single wide lane mask plus extracts would be affected. I don't think replacing an extract in this way would be valid however, when the wide lane mask has multiple uses because we are interleaving?
I can add a comment if you think it's worth making this clear though.
…terleaving Previously, when passing -enable-wide-lane-mask to request the use of wide lane masks, tryToReplaceALMWithWideALM would attempt to replace several ActiveLaneMasks created during unrolling with a single mask + extracts. This PR removes tryToReplaceALMWithWideALM from VPTransforms and makes wide lane masks the canonical form when interleaving a tail-folded loop which uses ActiveLaneMask for control flow. During unrolling, the multiplier is set to UF for ActiveLaneMask and one ExtractSubvectorForPart instruction for each part is created. Unnecessary ExtractSubvectorForParts are removed during the generate stage if the loop is not unrolled, otherwise this creates an extract subvector for each part. This does not change the default behaviour for tail-folded loops, which are still not considered for interleaving by default. The decision on whether to use a single wide active lane mask with extracts or to split it up into multiple smaller masks is now left to the backend.
…tract - Move ExtractSubvectorForPart unrolling to unrollRecipeByUF - Fix test failures
- Remove Op1/Op2
- Rename ExtractSubvectorForPart -> ExtractVectorForPart - Update comments in VPTransforms to reflect wide ALM + extracts
- Moved ICMP optimisation to optimizeForVFAndUF - Remove preferTailFoldedLoop from selectInterleaveCount() - Add test with i32 trip count
4223140 to
403bcaa
Compare
|
Friendly ping :) |
| // mask[i] = icmpt ult (op0 + i), op1 | ||
| // The size of the mask returned is VF * Multiplier (UF, third op). | ||
| // The size of the mask returned is VF. | ||
| // When unrolled, ActiveLaneMask is duplicated. |
There was a problem hiding this comment.
Is this still true given that WideActiveLaneMask is now the canonical form for unrolled, tail-folded loops? If WideActiveLaneMask isn't used as the canonical form for all loops, it would be good to explain in which scenarios unrolling using ActiveLaneMask occurs.
There was a problem hiding this comment.
WideActiveLaneMask is only the canonical form for unrolled, tail-folded loops if the style is DataAndControlFlow. For other styles, we still use ActiveLaneMask and will duplicate the mask (strict-fadd-interleave-only.ll does test this scenario, although the intrinsic is replaced with icmp because the VF is scalar). I've tried to rewrite the comment a bit to make this clearer though.
| static bool tryToReplaceALMWithWideALM(VPlan &Plan, ElementCount VF, | ||
| unsigned UF) { | ||
| if (!EnableWideActiveLaneMask || !VF.isVector() || UF == 1) | ||
| static bool replaceMaskWithCompare(VPlan &Plan, ElementCount BestVF) { |
There was a problem hiding this comment.
nit: Is it worth adding some comments explaining what this function is doing? It looks like an optimisation.
- Replace unsigned with uint64_t for getZExtValue() - Add comment to describe replaceMaskWithCompare
- Remove CanonicalIVIncrementForPart in addVPLaneMaskPhiAndUpdateExitBranch
and ensure isUniformAcrossVFsAndUFs returns false for ExtractVectorForPart
- Use overflowing add in replaceMaskWithCompare
- Remove `InLoopIncrement` from addVPLaneMaskPhiAndUpdateExitBranch - Revert unnecessary change to replaceMaskWithCompare
6 participants
Previously, when passing
-enable-wide-lane-maskto request the use of widelane masks,
tryToReplaceALMWithWideALMwould attempt to replace severalActiveLaneMasks created during unrolling with a single mask + extracts.
This PR removes
tryToReplaceALMWithWideALMfrom VPTransforms and makeswide lane masks the canonical form when interleaving a tail-folded loop
which uses ActiveLaneMask for control flow, as discussed on #193757.
During unrolling, the multiplier is set to UF for ActiveLaneMask and one
ExtractSubvectorForPart instruction for each part is created. Unnecessary
ExtractSubvectorForParts are removed during the generate stage if the loop
is not unrolled, otherwise this creates an extract subvector for each part.
This does not change the default behaviour for tail-folded loops, which
are still not considered for interleaving by default. The decision on
whether to use a single wide active lane mask with extracts or to split
it up into multiple smaller masks is now left to the backend.