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

Inline the layout-related methods as these basically change types and could benefit from cold branch removal. #332

Merged
merged 1 commit into from Jul 3, 2022

Conversation

adamreichold
Copy link
Member

No description provided.

@adamreichold adamreichold merged commit c0549bd into main Jul 3, 2022
@adamreichold adamreichold deleted the inline-layout-methods branch July 3, 2022 08:23
@davidhewitt
Copy link
Member

Sorry for the delayed review, this looks good to me 👍

I had stalled for a bit because these are generic functions and based off the discussion at PyO3/pyo3#2326 (comment) I wasn't sure if this achieves anything.

I just took a chance to play around with cargo rustc -- --emit=llvm-ir to look at what actually gets passed to llvm. It looks like the difference is that functions marked #[inline] carry the LLVM inlinehint attribute.

So my conclusion is that:

  • Because generic functions are instantiated in the calling crate they might get inlined anyway.
  • Adding #[inline] to generic functions can still have benefit because it provides an even stronger hint to LLVM to consider inlining.

Because they're generic functions this presumably also doesn't carry as much of a compile-time cost as marking a non-generic function #[inline]. The caller already has to codegen the concrete implementation, and the #[inline] attribute just adds a small hint to that codegen output.

So in summary, looks like there's definitely value in adding this.

cc @mejrs might be interesting to you too.

@adamreichold
Copy link
Member Author

Because they're generic functions this presumably also doesn't carry as much of a compile-time cost as marking a non-generic function #[inline]. The caller already has to codegen the concrete implementation, and the #[inline] attribute just adds a small hint to that codegen output.

I would try to summarize things by "Generic function are available for cross-crate inlining independently of the inline attribute, but being generic by itself does increase the likelihood of being inlined."

In the end, this change here is probably not affecting things too much either way as LLVM's infamous heuristic for inlining seems to be "yes"...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants