-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[LV] Use ExtractLane(LastActiveLane, V) live outs when tail-folding. #149042
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 11 commits
b7b23a9
9985387
6b0c9d2
2a45f94
7d1ab3c
8ca0426
96c827b
72f3796
f3fdac7
af82079
cc7e913
2d3012f
4fa7442
ae81e08
f10fa37
2758f96
73b3197
d8e77ca
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -44,11 +44,6 @@ class VPPredicator { | |||||
| /// possibly inserting new recipes at \p Dst (using Builder's insertion point) | ||||||
| VPValue *createEdgeMask(VPBasicBlock *Src, VPBasicBlock *Dst); | ||||||
|
|
||||||
| /// Returns the *entry* mask for \p VPBB. | ||||||
| VPValue *getBlockInMask(VPBasicBlock *VPBB) const { | ||||||
| return BlockMaskCache.lookup(VPBB); | ||||||
| } | ||||||
|
|
||||||
| /// Record \p Mask as the *entry* mask of \p VPBB, which is expected to not | ||||||
| /// already have a mask. | ||||||
| void setBlockInMask(VPBasicBlock *VPBB, VPValue *Mask) { | ||||||
|
|
@@ -68,6 +63,11 @@ class VPPredicator { | |||||
| } | ||||||
|
|
||||||
| public: | ||||||
| /// Returns the *entry* mask for \p VPBB. | ||||||
| VPValue *getBlockInMask(VPBasicBlock *VPBB) const { | ||||||
| return BlockMaskCache.lookup(VPBB); | ||||||
| } | ||||||
|
|
||||||
| /// Returns the precomputed predicate of the edge from \p Src to \p Dst. | ||||||
| VPValue *getEdgeMask(const VPBasicBlock *Src, const VPBasicBlock *Dst) const { | ||||||
| return EdgeMaskCache.lookup({Src, Dst}); | ||||||
|
|
@@ -301,5 +301,38 @@ VPlanTransforms::introduceMasksAndLinearize(VPlan &Plan, bool FoldTail) { | |||||
|
|
||||||
| PrevVPBB = VPBB; | ||||||
| } | ||||||
|
|
||||||
| // If we folded the tail and introduced a header mask, any extract of the | ||||||
| // last element must be updated to extract from the last active lane of the | ||||||
| // header mask instead (i.e., the lane corresponding to the last active | ||||||
| // iteration). | ||||||
| if (FoldTail) { | ||||||
| assert(Plan.getExitBlocks().size() == 1 && | ||||||
| "only a single-exit block is supported currently"); | ||||||
| VPBasicBlock *EB = Plan.getExitBlocks().front(); | ||||||
| assert(EB->getSinglePredecessor() == Plan.getMiddleBlock() && | ||||||
| "the exit block must have middle block as single predecessor"); | ||||||
|
|
||||||
| VPValue *LastActiveLane = nullptr; | ||||||
| VPBuilder B(Plan.getMiddleBlock()->getTerminator()); | ||||||
| for (auto &P : EB->phis()) { | ||||||
| auto *ExitIRI = cast<VPIRPhi>(&P); | ||||||
| VPValue *Inc = ExitIRI->getIncomingValue(0); | ||||||
| VPValue *Op; | ||||||
| if (!match(Inc, m_ExtractLastElement(m_VPValue(Op)))) | ||||||
| continue; | ||||||
|
|
||||||
| if (!LastActiveLane) { | ||||||
| // Compute the index of the last active lane. | ||||||
| VPValue *HeaderMask = Predicator.getBlockInMask( | ||||||
| Plan.getVectorLoopRegion()->getEntryBasicBlock()); | ||||||
| LastActiveLane = | ||||||
| B.createNaryOp(VPInstruction::LastActiveLane, {HeaderMask}); | ||||||
|
||||||
| B.createNaryOp(VPInstruction::LastActiveLane, {HeaderMask}); | |
| B.createNaryOp(VPInstruction::LastActiveLane, HeaderMask); |
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.
Yep, updated thanks
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -1965,6 +1965,33 @@ bool VPlanTransforms::adjustFixedOrderRecurrences(VPlan &Plan, | |||||
| // Set the first operand of RecurSplice to FOR again, after replacing | ||||||
| // all users. | ||||||
| RecurSplice->setOperand(0, FOR); | ||||||
|
|
||||||
| // Check for users extracting the second-to-last active lane of the FOR. If | ||||||
| // only a single lane is active in the current iteration, we need to select | ||||||
| // the last element of the value from the previous iteration, directly from | ||||||
| // the FOR phi. | ||||||
| for (VPUser *U : RecurSplice->users()) { | ||||||
| if (!match(U, | ||||||
| m_VPInstruction<VPInstruction::ExtractLane>( | ||||||
| m_LastActiveLane(m_VPValue()), m_Specific(RecurSplice)))) | ||||||
| continue; | ||||||
|
|
||||||
| VPBuilder B(cast<VPInstruction>(U)); | ||||||
| VPValue *LastActiveLane = cast<VPInstruction>(U)->getOperand(0); | ||||||
| Type *I64Ty = Type::getInt64Ty(Plan.getContext()); | ||||||
| VPValue *Zero = Plan.getOrAddLiveIn(ConstantInt::get(I64Ty, 0)); | ||||||
| VPValue *One = Plan.getOrAddLiveIn(ConstantInt::get(I64Ty, 1)); | ||||||
| VPValue *PenultimateIndex = | ||||||
| B.createNaryOp(Instruction::Sub, {LastActiveLane, One}); | ||||||
| VPValue *PenultimateLastIter = | ||||||
| B.createNaryOp(VPInstruction::ExtractLane, | ||||||
| {PenultimateIndex, FOR->getBackedgeValue()}); | ||||||
| VPValue *LastPrevIter = | ||||||
| B.createNaryOp(VPInstruction::ExtractLastElement, {FOR}); | ||||||
|
||||||
| B.createNaryOp(VPInstruction::ExtractLastElement, {FOR}); | |
| B.createNaryOp(VPInstruction::ExtractLastElement, FOR); |
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.
done thanks
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.
I think we have to be careful about this transformation because it's making the assumption that there are no holes in the mask. For example, you'd presumably get something unexpected if you transform LastActiveLane(11100111000) -> FirstActiveLane(00011000111).
It might require adding documentation to the opcodes saying that holes are not permitted when using the LastActiveLane opcode. I don't know if there is a way to add an assert here that the mask doesn't contain holes? Presumably it should only be used when the mask has originally come get.active.lane.mask?
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.
Yep, for now the operands will always be header masks (or EVL based masks), so we can verify that (added to VPlanVerifier). Updated and extended the comment for the opcode, thanks
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.
So I've added a similar opcode (ExtractLastActive) as part of https://github.com/llvm/llvm-project/pull/158088/files#diff-dabd8bf9956285f5ddab712af4e0494647a106234f07283b0bd9f8b4d9b887ca , and that does expect that there might be holes.
Should we keep the opcodes separate, or express one in terms of the other and enforce the 'contiguousness' of active lanes directly for your code?
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.
Having the LastActiveLane check separate has the potential advantage that it could expose more CSE/simplification opportuntiies. The restriction for prefix-masks at the moment is only there due to how it gets lowered using FirstActiveLane.
I think we can just remove the restriction once we have a use-case for non-prefix-masks and add dedicated lowering that supports them.
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.
I really can't think of when we'd end up using LastActiveLane with anything but the header mask.
Would it be simpler to just make LastActiveLane a nullary op, and just fetch the header mask with findHeaderMask when converting to a concrete recipe? That way we don't have to worry about the non-contiguous invariant.
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.
Unfortunately I don't think that's possible at the moment, as at the point when we call covnertToConcreteRecipes, the region may have been removed (or if the LastActiveLane would be the only user of the header mask it may have been removed), so I've left it as-is for now
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.
I'm also realising now that we need to model the use of the header mask to prevent removeDeadRecipes from stripping it if e.g. it's EVL tail folded and all other uses of it are removed.
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -18,6 +18,7 @@ | |||||
| #include "VPlanDominatorTree.h" | ||||||
| #include "VPlanHelpers.h" | ||||||
| #include "VPlanPatternMatch.h" | ||||||
| #include "VPlanUtils.h" | ||||||
| #include "llvm/ADT/SmallPtrSet.h" | ||||||
| #include "llvm/ADT/TypeSwitch.h" | ||||||
|
|
||||||
|
|
@@ -44,6 +45,9 @@ class VPlanVerifier { | |||||
| /// incoming value into EVL's recipe. | ||||||
| bool verifyEVLRecipe(const VPInstruction &EVL) const; | ||||||
|
|
||||||
| /// Verify that \p LastActiveLane's operand is guaranteed to be a prefix-mask. | ||||||
| bool verifyLastActiveLaneRecipe(const VPInstruction &LastActiveLane) const; | ||||||
|
|
||||||
| bool verifyVPBasicBlock(const VPBasicBlock *VPBB); | ||||||
|
|
||||||
| bool verifyBlock(const VPBlockBase *VPB); | ||||||
|
|
@@ -221,6 +225,44 @@ bool VPlanVerifier::verifyEVLRecipe(const VPInstruction &EVL) const { | |||||
| }); | ||||||
| } | ||||||
|
|
||||||
| bool VPlanVerifier::verifyLastActiveLaneRecipe( | ||||||
| const VPInstruction &LastActiveLane) const { | ||||||
| assert(LastActiveLane.getOpcode() == VPInstruction::LastActiveLane && | ||||||
| "must be called with VPInstruction::LastActiveLane"); | ||||||
|
|
||||||
| if (LastActiveLane.getNumOperands() < 1) { | ||||||
| errs() << "LastActiveLane must have at least one operand\n"; | ||||||
| return false; | ||||||
| } | ||||||
|
|
||||||
| const VPlan &Plan = *LastActiveLane.getParent()->getPlan(); | ||||||
| // All operands must be prefix-mask. Currently we check for header masks or | ||||||
| // EVL-derived masks, as those are currently the only operands in practice, | ||||||
| // but this may need updating in the future.. | ||||||
|
||||||
| // but this may need updating in the future.. | |
| // but this may need updating in the future. |
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.
done thanks
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 the PR handle the IV liveout user as well?
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 patch still leaves inductions untouched for now