Page MenuHomePhabricator

[SimpleLoopUnswitch] Don't non-trivially unswitch loops with catchswitch exits
ClosedPublic

Authored by aeubanks on Jul 8 2021, 4:32 PM.

Details

Summary

SplitBlock() can't handle catchswitch.

Fixes PR50973.

Diff Detail

Event Timeline

aeubanks created this revision.Jul 8 2021, 4:32 PM
aeubanks requested review of this revision.Jul 8 2021, 4:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2021, 4:32 PM
aheejin accepted this revision.Jul 8 2021, 4:45 PM

Thank you!

llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
2784

Oh, I didn't find there's already a similar condition for cleanuppad...

This revision is now accepted and ready to land.Jul 8 2021, 4:45 PM
majnemer requested changes to this revision.Jul 8 2021, 7:21 PM
majnemer added a subscriber: majnemer.
majnemer added inline comments.
llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
2778–2787

Why is splitting a block with a cleanuppad different from a block with a landingpad?

This revision now requires changes to proceed.Jul 8 2021, 7:21 PM
aheejin added inline comments.Jul 8 2021, 7:40 PM
llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
2778–2787

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.

majnemer added inline comments.Jul 8 2021, 9:33 PM
llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
2778–2787

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?

aheejin added inline comments.Jul 9 2021, 2:51 AM
llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
2778–2787

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.

aeubanks added inline comments.Jul 9 2021, 8:49 AM
llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
2778–2787

The added assert in SplitBlockImpl() is triggering on the block with the catchswitch. SplitBlockImpl() assumes that there is a non-PHI/non-EHPad instruction after split point instruction, which is not true for catchswitch

rnk added a comment.Jul 9 2021, 9:00 AM

By the way, the catchswitch instructions true purpose is really just to multiplex invokes. When we added WinEH, callbr didn't exist. We felt that extending invoke with multiple unwind edges would be too disruptive. If we were doing it today, we would probably implement this in such a way that blocks without insertion points do not exist. That's a good project that would simplify LLVM transforms in the long run. It's probably not that straightforward, since most passes think callbr is just a wrapper for asm goto.

The cleanuppad design is probably as good as it's going to get. If we want to unswitch a loop inside a destructor cleanup, we wouldn't want that pass to accidentally create two prologues for a cleanup funclet, that would be too challenging. Instead, passes that really need to insert code along edges entering a cleanuppad can be taught to create new trivial cleanup funclets. This isn't too different from splitting edges coming into a landingpad, which LLVM knows how to do.

any remaining objections?

Can we land this? @majnemer, Do you have any remaining concerns?

@aeubanks, Can we land this? Thank you!

This revision was not accepted when it landed; it landed in state Needs Revision.Jul 14 2021, 2:14 PM
This revision was automatically updated to reflect the committed changes.