[VPlan] Don't collect live-ins in collectUsersInExitBlocks. (NFC) #123819
Conversation
|
@llvm/pr-subscribers-vectorizers @llvm/pr-subscribers-llvm-transforms Author: Florian Hahn (fhahn) ChangesLive-ins don't need to be handled, other than adding to the exit phi recipe. Do that early and assert that otherwise the exit value is defined in the vector loop region. This should enable simply skipping other exit values that do not need further fixing, e.g. if handling the exit value from the early exit directly in handleUncountableEarlyExit. Full diff: https://github.com/llvm/llvm-project/pull/123819.diff 1 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index df64bb2884ab88..68406ecdff096c 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -9052,8 +9052,12 @@ collectUsersInExitBlocks(Loop *OrigLoop, VPRecipeBuilder &Builder,
}
Value *IncomingValue = ExitPhi->getIncomingValueForBlock(ExitingBB);
VPValue *V = Builder.getVPValueOrAddLiveIn(IncomingValue);
- ExitUsersToFix.insert(ExitIRI);
ExitIRI->addOperand(V);
+ if (V->isLiveIn())
+ continue;
+ assert(V->getDefiningRecipe()->getParent()->getEnclosingLoopRegion() &&
+ "Only recipes defined inside a region should need fixing.");
+ ExitUsersToFix.insert(ExitIRI);
}
}
}
@@ -9077,11 +9081,6 @@ addUsersInExitBlocks(VPlan &Plan,
// modeling the corresponding LCSSA phis.
for (VPIRInstruction *ExitIRI : ExitUsersToFix) {
for (const auto &[Idx, Op] : enumerate(ExitIRI->operands())) {
- // Pass live-in values used by exit phis directly through to their users
- // in the exit block.
- if (Op->isLiveIn())
- continue;
-
// Currently only live-ins can be used by exit values from blocks not
// exiting via the vector latch through to the middle block.
if (ExitIRI->getParent()->getSinglePredecessor() != MiddleVPBB)
|
| @@ -9077,11 +9081,6 @@ addUsersInExitBlocks(VPlan &Plan, | |||
| // modeling the corresponding LCSSA phis. | |||
| for (VPIRInstruction *ExitIRI : ExitUsersToFix) { | |||
| for (const auto &[Idx, Op] : enumerate(ExitIRI->operands())) { | |||
| // Pass live-in values used by exit phis directly through to their users | |||
| // in the exit block. | |||
There was a problem hiding this comment.
Can you hold off on this patch for a bit until I've tested the interaction with #120567 please? I'm worried this essentially means that there is no sensible way to get hold of the predecessor.
There was a problem hiding this comment.
Yes definitely the patch tries to help simplify #120567, so we should only land this if it helps.
There was a problem hiding this comment.
OK, I think I now see what you're doing here. I thought this was just a temporary solution to get at least one test working, but it sounds like you want to take the approach of duplicating the code in addUsersInExitBlocks for the latch exit and hide the live-outs from the ExitUsersToFix list in LoopVectorize.cpp. This will work, but my concern here is that the code in handleUncountableEarlyExit may get left behind when fixes/improvements are made for loops without early exits. ExitUsersToFix is also passed in to addExitUsersForFirstOrderRecurrences and I can see in future that list might be used for something else. Although it will probably be a while before support is added for first order recurrences, it might mean this code has to be rewritten at some point.
I understand what you mean in your comment on #120567 about this avoiding the problem of ordering when calling collectUsersInExitBlocks, followed by handleUncountableEarlyExit. I think if we go with this solution I'd prefer to see both addUsersInEarlyExitBlock and handleUncountableEarlyExit call a common function to manage the live-out, i.e. pull this code out:
VPValue *Ext = B.createNaryOp(VPInstruction::ExtractFromEnd,
{Op, Plan.getOrAddLiveIn(ConstantInt::get(
IntegerType::get(Ctx, 32), 1))});
ExitIRI->setOperand(Idx, Ext);
That way anyone modifying this code is forced to take early exit loops into consideration too. There is already an assert in addExitUsersForFirstOrderRecurrences that there is no early exit so at least that is defended.
Apologies if I am being a little paranoid here, but until the early exit work is enabled by default I just want to avoid something getting broken along the way. :)
There was a problem hiding this comment.
Sounds good, I added VPIRInstruction::extractLastLaneOfOperand, is that along the lines you had in mind?
Now that all live-outs are handled in VPlan I am planning to look into simplifying and unifying the place where we handle the exit users. If/once there's support for reductions/first-order recurrences for early exit loops, I think they should ideally also handled directly in handleUncountableEarlyExit, but that's for another day :)
Live-ins don't need to be handled, other than adding to the exit phi recipe. Do that early and assert that otherwise the exit value is defined in the vector loop region. This should enable simply skipping other exit values that do not need further fixing, e.g. if handling the exit value from the early exit directly in handleUncountableEarlyExit.
fhahn
force-pushed
the
vplan-handle-live-in-early
branch
from
0f992a3 to
3d30a34
Compare
Sounds great, thanks! |
…. (NFC) (#123819) Live-ins don't need to be handled, other than adding to the exit phi recipe. Do that early and assert that otherwise the exit value is defined in the vector loop region. This should enable simply skipping other exit values that do not need further fixing, e.g. if handling the exit value from the early exit directly in handleUncountableEarlyExit. PR: llvm/llvm-project#123819
Live-ins don't need to be handled, other than adding to the exit phi recipe. Do that early and assert that otherwise the exit value is defined in the vector loop region.
This should enable simply skipping other exit values that do not need further fixing, e.g. if handling the exit value from the early exit directly in handleUncountableEarlyExit.