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

Avoid link-time dependency on core through to_bits and from_bits #472

Closed
wants to merge 1 commit into from
Closed

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Jun 13, 2022

The compiler-builtins should not have any link time dependencies on any
other crates including core.

Replace uses of to_bits and from_bits from core with local repr
and to_repr methods implemented in terms of the transmute intrinsic.

This fixes a build issue that can occur in build configurations that
share generic monomorphizations between crates.

The compiler-builtins should not have any link time dependencies on any
other crates including core.

Replace uses of `to_bits` and `from_bits` from core with local `repr`
and `to_repr` methods implemented in terms of the `transmute` intrinsic.

This fixes a build issue that can occur in build configurations that
share generic monomorphizations between crates.
@bjorn3
Copy link
Member

bjorn3 commented Jun 13, 2022

These functions should always be inlined, right? They are marked as #[inline] and small enough that no reasonable compiler would not inline them when optimizations are enabled.

@tmiasko
Copy link
Contributor Author

tmiasko commented Jun 13, 2022

Those functions themselves are indeed marked inline but not everything they transitively use is.

@bjorn3
Copy link
Member

bjorn3 commented Jun 13, 2022

What do you mean? f*::to_bits and f*::from_bits are both marked as #[inline] and only call mem::transmute which is an intrinsic and thus by definition inlined.

@tmiasko
Copy link
Contributor Author

tmiasko commented Jun 13, 2022

to_bits calls const_eval_select, which calls call_once, which calls a closure, which calls the intrinsic.

@bjorn3
Copy link
Member

bjorn3 commented Jun 13, 2022

I see. On stable it is a simple transmute. On nightly it uses const_eval_select. Making the rt_f32_to_u32 closure a regular function and marking it as #[inline] should help with that I think.

@tmiasko
Copy link
Contributor Author

tmiasko commented Jun 13, 2022

Given the role of compiler builtins, I would assume that limiting the dependency on the core is desirable regardless, so this was my first choice in terms of the implementation strategy. Modifying the core instead would presumably work as well.

@Amanieu
Copy link
Member

Amanieu commented Jun 13, 2022

We can't realistically avoid all dependencies on core, we just have to make sure that we only use inline functions. It is concerning however that to_bits/from_bits is not properly inlined in core and this should be fixed there.

@tmiasko tmiasko closed this Jun 13, 2022
@tmiasko tmiasko deleted the no-core branch June 13, 2022 14:47
@tmiasko
Copy link
Contributor Author

tmiasko commented Jun 13, 2022

I opened a pull request to modify the core instead. Though, that approach is not as robust as solution proposed here since it requires enabled optimizations when buliding the core.

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.

3 participants