[PATCH] D105672: [SimpleLoopUnswitch] Don't non-trivially unswitch loops with catchswitch exits

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2778-2784
+  // We cannot unswitch if exit blocks contain a cleanuppad/catchswitch
+  // instruction as we don't know how to split those exit blocks.
   // FIXME: We should teach SplitBlock to handle this and remove this
   // restriction.
   for (auto *ExitBB : ExitBlocks) {
-    if (isa<CleanupPadInst>(ExitBB->getFirstNonPHI())) {
-      LLVM_DEBUG(
-          dbgs() << "Cannot unswitch because of cleanuppad in exit block\n");
+    auto *I = ExitBB->getFirstNonPHI();
+    if (isa<CleanupPadInst>(I) || isa<CatchSwitchInst>(I)) {
----------------
majnemer wrote:
> aheejin wrote:
> > majnemer wrote:
> > > Why is splitting a block with a cleanuppad different from a block with a landingpad?
> > The splitting here is to split the BB into BB with the first instruction and BB the rest and clone the BB with the first instruction, which happens when the EH pad BB is among a loop's exit blocks. SimpleUnswitch clones the loop and all its exit blocks, but instead of cloning the full exit blocks, it splits those blocks into the first instruction and the rest and clone only the first instruction. So
> > 
> > Before:
> > ```
> > bb0:
> >   first instruction
> >   second instruction
> >   ...
> >   br %succ
> > 
> > succ:
> >   ...
> > ```
> > 
> > If `bb0` is among the exit blocks of a loop that's being unswitched,
> > ```
> > bb0:
> >   first instruction
> >   br %bb0.rest
> > 
> > bb0.split:
> >   first instruction
> >   br %bb0.rest
> > 
> > bb0.rest:
> >   second instruction
> >   ...
> >   br %succ
> > 
> > succ:
> >   ...
> > ```
> > 
> > If `bb0` contains `catchswitch` or `cleanuppad`, it is cloned because it is the first instruction, and its token return value should be merged with a `phi` in `bb0.rest`. But tokens can't be phi'd, right?
> > 
> > This is the reason I thought why they are different from `landingpad`, but I can be mistaken, so please let me know if so.
> OK, so it does not have to do with SplitBlock. In that case, I'd adjust the comment to make it more clear.
> 
> So if the issue is that it needs to do something about tokens which will now be live-out of the block, this seems unrelated to ehpads and more about token producing instructions/intrinsics. IIRC, I tried to handle this here: https://github.com/llvm/llvm-project/blob/873ff5a72864fdf60614cca8adbd0d869fc9a9a2/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp#L2819-L2820
> 
> Maybe I got it wrong?
In the attached test here, `catchswitch` is not inside the loop; it is an exit block of the loop. The code snippet you showed seems to only count for instructions within a loop.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105672/new/

https://reviews.llvm.org/D105672