Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cold continuation handlers in flambda2 #1543

Merged
merged 8 commits into from
Aug 4, 2023

Conversation

Ekdohibs
Copy link
Contributor

@Ekdohibs Ekdohibs commented Jul 4, 2023

Cold continuation handlers will not be inlined, neither by simplify, nor by to_cmm. For now, they are generated only in from_lambda for the exception handlers of checked operations, but the plan is to add then in lambda as well and propagate the information. The backend parts seem more complex however: while it seems to be easy enough to propagate the information into Cmm code, it's unclear how to represent it inside Mach and Linear if we want to be able to put cold continuation handlers at the end of the function.

@mshinwell mshinwell added the flambda2 Prerequisite for, or part of, flambda2 label Jul 4, 2023
@Ekdohibs Ekdohibs marked this pull request as ready for review July 6, 2023 09:57
@Ekdohibs
Copy link
Contributor Author

Ekdohibs commented Jul 6, 2023

This now includes propagation of is_cold to Cmm and Mach, and is ready for review; backend support (at least Cfg) to ensure cold continuation handlers are compiled in a different section or at the end of the function will come in a separate PR.
Some things I'd like to be given some thought about this PR:

  • Cmm and Mach handlers are becoming large tuples, and this PR needs to change every single place they are used causing a large diff. Should we change them to a record instead?
  • Can we try to use the coldness information is spill/reload to say that the cost of reloading a register on cold paths is reduced?

@gretay-js
Copy link
Contributor

  • Cmm and Mach handlers are becoming large tuples, and this PR needs to change every single place they are used causing a large diff. Should we change them to a record instead?

I also think it should be a record, at least for the handlers. I think the change to a record will make the diff in this PR bigger though, so maybe a separate PR would actually be easier to review.

  • Can we try to use the coldness information is spill/reload to say that the cost of reloading a register on cold paths is reduced?

yes, I think the cfg-based register allocators are well suited for using this info.
What would be particularly useful for spill/reload placement is to propagate to the backend which calls are cold, based on user-provided [@inline never][@specialise never] annotation or even better provide compiler suport for @cold attribute and propagate that.

Copy link
Contributor

@gretay-js gretay-js left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only for files owned by me.

@Gbury
Copy link
Contributor

Gbury commented Aug 2, 2023

I agree that we should introduce some records for the Cmm.Ccatch case because it's becoming way too big of a tuple/list nested type, and that it should be done in a separate PR.

middle_end/flambda2/simplify/expr_builder.ml Outdated Show resolved Hide resolved
middle_end/flambda2/simplify/simplify_common.ml Outdated Show resolved Hide resolved
middle_end/flambda2/simplify/simplify_expr.ml Show resolved Hide resolved
middle_end/flambda2/simplify/simplify_let_cont_expr.ml Outdated Show resolved Hide resolved
middle_end/flambda2/simplify/simplify_let_cont_expr.ml Outdated Show resolved Hide resolved
@Ekdohibs Ekdohibs force-pushed the cold-continuations branch from 512d798 to 1c1f5f9 Compare August 4, 2023 12:54
Copy link
Contributor

@Gbury Gbury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to merge, ^^

@mshinwell mshinwell merged commit b212683 into ocaml-flambda:main Aug 4, 2023
Ekdohibs added a commit to Ekdohibs/flambda-backend that referenced this pull request Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flambda2 Prerequisite for, or part of, flambda2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants