[llvm] [VPlan] Don't collect live-ins in collectUsersInExitBlocks. (NFC) (PR #123819)

David Sherwood via llvm-commits llvm-commits at lists.llvm.org
Thu Jan 23 01:52:32 PST 2025


================
@@ -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.
----------------
david-arm wrote:

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. :)

https://github.com/llvm/llvm-project/pull/123819


More information about the llvm-commits mailing list