Page MenuHomePhabricator

[CodeGen] Split out cold exception handling pads.

Authored by snehasish on Feb 9 2021, 2:25 PM.



Support for splitting exception handling pads was added in D73739. This
change updates the code to split out exception handling pads if profile
information indicates that they are cold.

Diff Detail

Event Timeline

snehasish created this revision.Feb 9 2021, 2:25 PM
snehasish requested review of this revision.Feb 9 2021, 2:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 9 2021, 2:25 PM

Wouldn't this cause an error when we have both hot and cold eh pads? We must ensure that same section id is assigned to all landing pads (please refer to BasicBlockSections.cpp::assignSections.

snehasish updated this revision to Diff 322590.Feb 9 2021, 9:39 PM

Extend the check to account of multiple landing pads with difference in hotness.
Add a new test to verify the expected behaviour.

Yes, this would cause an issue if we had hot and cold landing pads. I've updated the logic to account for this and added a test. PTAL, thanks!

I tried this patch on an instrumented build of mysql server. With this patch, an additional 28KB of text is split out from hot functions, a small but non-trivial improvement considering the size of the section is ~1MB.

LGTM with minor nits.


"Use auto & for values and auto * for pointers unless you need to make a copy."
You might as well consider spelling out the type here since the type is not really obvious from this line.


Same as above.


nit: You can reuse this for the previous test case (the case where all landing pads are cold). You can still have two landing pads and make one of them configurable with a macro.

  %2 = call i32 @bar()
  invoke void @_Z1fv()
          to label %exit unwind label %lpad2, !prof ![[PROF]]

Just a suggestion. Using macros could make tests less readable too.

snehasish updated this revision to Diff 322888.Feb 10 2021, 6:25 PM

Spell out types instead of auto.

snehasish marked 2 inline comments as done.Feb 10 2021, 6:27 PM

Thanks for the review.


It's an interesting idea but I had a couple of concerns so I left it as is

  • It's really easy to miss the macro since the substitution is for the profile metadata
  • We would need to invoke llc again with two version of the macro
rahmanl accepted this revision.Feb 10 2021, 6:32 PM
rahmanl added inline comments.

SGTM. Thanks for considering.

This revision is now accepted and ready to land.Feb 10 2021, 6:32 PM
This revision was landed with ongoing or failed builds.Feb 11 2021, 11:25 AM
This revision was automatically updated to reflect the committed changes.