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

Feature/static dispatch #12

Merged
merged 2 commits into from
Nov 10, 2019
Merged

Feature/static dispatch #12

merged 2 commits into from
Nov 10, 2019

Conversation

calebzulawski
Copy link
Owner

@calebzulawski calebzulawski commented Nov 3, 2019

This is the final result of what I think is the solution for static dispatching. It has a few compromises, from what I can tell.

  • The function versions for a safe function are marked safe for each architecture, even though they are not. I don't particularly like this, but it allows static dispatch to inline correctly. They are marked with #[doc(hidden)] and given names that are unlikely to be shadowed, so unless a user seeks out the implementation details I don't think it's likely for this to cause problems.
  • The dynamically dispatched function doesn't inline perfectly. This results in an extra jump instruction after the indirect function call. I believe this is due to the functions now being marked safe and the compiler avoiding speculative execution of instruction set extensions.
  • Inlining recursion now requires using #[static_dispatch]. Personally I think it's more consistent this way, anyway.

I think these compromises are worth it in exchange for being able to statically dispatch and inline nested functions. @TethysSvensson I'm curious what your opinion is. I'm comfortable with the implementation if you just want to take a look with cargo expand/asm.

@calebzulawski calebzulawski merged commit 35f1372 into master Nov 10, 2019
@calebzulawski calebzulawski deleted the feature/static-dispatch branch November 10, 2019 14:11
@TethysSvensson
Copy link
Contributor

I'm sorry I missed this and didn't reply. I know this is already merged, but I'll take a look know if it's any consolation.

@TethysSvensson
Copy link
Contributor

I again haven't actually looked at how the proc_macro code works, just what it outputs.

I think what it outputs makes a lot of sense. I agree that it is sad that we cannot make the function unsafe, but also agree that it doesn't matter too much in practice.

I do not currently have time to go into detail about what the assembly output looks like, but perhaps I'll come back and give this crate another look at some point.

@calebzulawski
Copy link
Owner Author

Thanks for taking a look, I appreciate a second set of eyes!

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.

2 participants