-
Notifications
You must be signed in to change notification settings - Fork 18.3k
[LV] Use wide lane masks as the canonical form when tail-folding & interleaving #209484
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
337b77e
0733140
536da07
23b783f
a588ea0
5d3e353
380e4a6
38af60a
6a3a19d
d535d6f
c51aea1
d4a5095
764042d
8ea1f3b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1242,8 +1242,17 @@ class LLVM_ABI_FOR_TEST VPInstruction : public VPRecipeWithIRFlags, | |
| // 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 is used for tail-folding, with the exception of the | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: It is also used for early-exit loops with stores |
||
| // DataAndControlFlow style. The size of the mask returned is VF. | ||
| // When unrolled, ActiveLaneMask is duplicated. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this still true given that WideActiveLaneMask is now the canonical form for unrolled, tail-folded loops? If
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| ActiveLaneMask, | ||
| // As above, but takes an additional operand (Multiplier). The size of | ||
| // the mask returned is VF * Multiplier (UF, op2). | ||
| // WideActiveLaneMask is used for control flow and is unrolled by widening, | ||
| // with one extract vector created per unroll part. | ||
| WideActiveLaneMask, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please document to make clear what the difference to regular
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a new comment for |
||
| // Extracts each unrolled part of a (VF * UF) widened vector/mask. | ||
| ExtractVectorForPart, | ||
| ExplicitVectorLength, | ||
| // Represents the incoming loop-invariant alias-mask. All memory accesses | ||
| // in the loop must stay within the active lanes. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -118,7 +118,7 @@ void VPlanTransforms::replaceWideCanonicalIVWithWideIV( | |
|
|
||
| // Add a VPActiveLaneMaskPHIRecipe and related recipes to \p Plan and replace | ||
| // the loop terminator with a branch-on-cond recipe with the negated | ||
| // active-lane-mask as operand. Note that this turns the loop into an | ||
| // wide-active-lane-mask as operand. Note that this turns the loop into an | ||
| // uncountable one. Only the existing terminator is replaced, all other existing | ||
| // recipes/users remain unchanged, except for poison-generating flags being | ||
| // dropped from the canonical IV increment. Return the created | ||
|
|
@@ -128,15 +128,18 @@ void VPlanTransforms::replaceWideCanonicalIVWithWideIV( | |
| // | ||
| // vector.ph: | ||
| // %EntryInc = canonical-iv-increment-for-part CanonicalIVStart | ||
| // %EntryALM = active-lane-mask %EntryInc, TC | ||
| // %EntryALM = wide-active-lane-mask %EntryInc, TC | ||
| // %EntryALMPart = extract-vector-for-part %EntryALM, ir<0> | ||
| // | ||
| // vector.body: | ||
| // ... | ||
| // %P = active-lane-mask-phi [ %EntryALM, %vector.ph ], [ %ALM, %vector.body ] | ||
| // %P = active-lane-mask-phi [ %EntryALMPart, %vector.ph ], | ||
| // [ %ALMPart, %vector.body ] | ||
| // ... | ||
| // %InLoopInc = canonical-iv-increment-for-part CanonicalIVIncrement | ||
| // %ALM = active-lane-mask %InLoopInc, TC | ||
| // %Negated = Not %ALM | ||
| // %ALM = wide-active-lane-mask %InLoopInc, TC | ||
| // %ALMPart = extract-vector-for-part %ALM, ir<0> | ||
| // %Negated = Not %ALMPart | ||
| // branch-on-cond %Negated | ||
| // | ||
| static VPActiveLaneMaskPHIRecipe * | ||
|
|
@@ -148,29 +151,22 @@ addVPLaneMaskPhiAndUpdateExitBranch(VPlan &Plan) { | |
| // TODO: Check if dropping the flags is needed. | ||
| TopRegion->clearCanonicalIVNUW(CanonicalIVIncrement); | ||
| DebugLoc DL = CanonicalIVIncrement->getDebugLoc(); | ||
| // We can't use StartV directly in the ActiveLaneMask VPInstruction, since | ||
| // we have to take unrolling into account. Each part needs to start at | ||
| // Part * VF | ||
| auto *VecPreheader = Plan.getVectorPreheader(); | ||
| VPBuilder Builder(VecPreheader); | ||
|
|
||
| // Create the ActiveLaneMask instruction using the correct start values. | ||
| VPValue *TC = Plan.getTripCount(); | ||
| VPValue *VF = &Plan.getVF(); | ||
|
|
||
| auto *EntryIncrement = | ||
| Builder.createOverflowingOp(VPInstruction::CanonicalIVIncrementForPart, | ||
| {StartV, VF}, {}, DL, "index.part.next"); | ||
|
|
||
| // Create the active lane mask instruction in the VPlan preheader. | ||
| // Create the wide active lane mask instruction in the VPlan preheader. | ||
| VPValue *ALMMultiplier = | ||
| Plan.getConstantInt(TopRegion->getCanonicalIVType(), 1); | ||
| auto *EntryALM = Builder.createNaryOp(VPInstruction::ActiveLaneMask, | ||
| {EntryIncrement, TC, ALMMultiplier}, DL, | ||
| auto *EntryALM = Builder.createNaryOp(VPInstruction::WideActiveLaneMask, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm happy to take a look at |
||
| {StartV, TC, ALMMultiplier}, DL, | ||
| "active.lane.mask.entry"); | ||
| EntryALM = Builder.createNaryOp(VPInstruction::ExtractVectorForPart, | ||
|
kmclaughlin-arm marked this conversation as resolved.
|
||
| {EntryALM, Plan.getConstantInt(64, 0)}, DL, | ||
| "extract.entry.alm.part"); | ||
|
|
||
| // Now create the ActiveLaneMaskPhi recipe in the main loop using the | ||
| // preheader ActiveLaneMask instruction. | ||
| // preheader WideActiveLaneMask instruction. | ||
| auto *LaneMaskPhi = | ||
| new VPActiveLaneMaskPHIRecipe(EntryALM, DebugLoc::getUnknown()); | ||
| auto *HeaderVPBB = TopRegion->getEntryBasicBlock(); | ||
|
|
@@ -180,12 +176,12 @@ addVPLaneMaskPhiAndUpdateExitBranch(VPlan &Plan) { | |
| // original terminator. | ||
| VPRecipeBase *OriginalTerminator = EB->getTerminator(); | ||
| Builder.setInsertPoint(OriginalTerminator); | ||
| auto *InLoopIncrement = Builder.createOverflowingOp( | ||
| VPInstruction::CanonicalIVIncrementForPart, | ||
| {CanonicalIVIncrement, &Plan.getVF()}, {}, DL); | ||
| auto *ALM = Builder.createNaryOp(VPInstruction::ActiveLaneMask, | ||
| {InLoopIncrement, TC, ALMMultiplier}, DL, | ||
| "active.lane.mask.next"); | ||
| auto *ALM = Builder.createNaryOp(VPInstruction::WideActiveLaneMask, | ||
| {CanonicalIVIncrement, TC, ALMMultiplier}, | ||
| DL, "active.lane.mask.next"); | ||
| ALM = Builder.createNaryOp(VPInstruction::ExtractVectorForPart, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
| {ALM, Plan.getConstantInt(64, 0)}, DL, | ||
| "extract.next.alm.part"); | ||
| LaneMaskPhi->addBackedgeValue(ALM); | ||
|
|
||
| // Replace the original terminator with BranchOnCond. We have to invert the | ||
|
|
@@ -215,12 +211,9 @@ void VPlanTransforms::materializeHeaderMask( | |
| VPIRFlags::WrapFlagsTy(/*HasNUW=*/true, /*HasNSW=*/false))); | ||
| VPValue *Mask; | ||
| if (UseActiveLaneMask) { | ||
| VPValue *ALMMultiplier = | ||
| Plan.getConstantInt(LoopRegion->getCanonicalIVType(), 1); | ||
| Mask = Builder.createNaryOp( | ||
| VPInstruction::ActiveLaneMask, | ||
| {WideCanonicalIV, Plan.getTripCount(), ALMMultiplier}, nullptr, | ||
| "active.lane.mask"); | ||
| Mask = Builder.createNaryOp(VPInstruction::ActiveLaneMask, | ||
| {WideCanonicalIV, Plan.getTripCount()}, nullptr, | ||
| "active.lane.mask"); | ||
| } else { | ||
| Mask = Builder.createICmp(CmpInst::ICMP_ULE, WideCanonicalIV, | ||
| Plan.getOrCreateBackedgeTakenCount()); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this change behavior for other tail folding styles that do not use ALM?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this just reverting back to the original code before
useWideActiveLaneMaskwas introduced?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was intended to revert
selectInterleaveCountback to the original code before #163387, howeverCM.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.