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

Fix passing target-cpu and codegen-units to dependencies #356

Merged
merged 1 commit into from
Dec 23, 2024

Conversation

zheland
Copy link
Contributor

@zheland zheland commented Dec 23, 2024

Fixes #355

Changes

This PR fixes passing target-cpu to the dependencies using RUSTFLAGS instead of cargo rustc args.

Other arguments

  • user-supplied codegen flags - okay with cargo rustc args.
  • asm syntax - okay with cargo rustc args.
  • target-cpu - should be passed as RUSTFLAGS to compile deps with target-cpu.
  • debuginfo - probably don't affect deps, so okay with cargo rustc args.
  • codegen-units - probably should be passed as RUSTFLAGS to be applicable for deps, but I am not sure. Please check this. If passed with RUSTFLAGS may overwrite user-specified codegen-units in RUSTFLAGS.

Notes

  • RUSTFLAGS are prioritized over cargo rustc args, so cargo asm --native or cargo asm --target-cpu=CPU will now overwrite user-specified target-cpu in RUSTFLAGS.
  • Using write!(...).unwrap(); to &mut String seems to be fine and already used in code.

This fixes passing `target-cpu` and `codegen-units` to dependencies or
to library if invocated for examples or benches.

`args` from `cargo rustc -- args` are passed only to the final compiler
instance. `RUSTFLAGS` envvar is useful for passing flags to all compiler
instances.
@pacak
Copy link
Owner

pacak commented Dec 23, 2024

Makes sense. Initially I thought that it doesn't really matter how dependencies are compiled, but your example with benchmarks is pretty convincing.

As far as codegen units are concerned - I'm setting them to 1 just to make life easier figuring out what .s file we should be using. I submitted changes to rustc (rust-lang/rust#122597), but things are stuck behind cargo giving access to this info (rust-lang/cargo#13672). There are some flags to cargo that would make it print the right invocation of rustc so we can run it directly and access the output.

debuginfo - used to attribute asm lines to rust lines, won't change the compilation result...

Anyway, I think your change is good to be merged as is. Thanks again for your contribution ❤️

@pacak pacak merged commit 178357b into pacak:master Dec 23, 2024
3 of 6 checks passed
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.

Target cpu option is passed only to the final compiler target, not to its deps
2 participants