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

Expose target-cpu/target-feature #1137

Closed
huonw opened this issue Jan 9, 2015 · 17 comments
Closed

Expose target-cpu/target-feature #1137

huonw opened this issue Jan 9, 2015 · 17 comments

Comments

@huonw
Copy link
Member

huonw commented Jan 9, 2015

rustc won't use e.g. AVX instructions without being told that it's ok to compile to a non-lowest-common-denominator CPU, and programmers may wish to opt-in to these more specialised instructions.

@huonw
Copy link
Member Author

huonw commented Jan 29, 2015

cc #544

@alexcrichton
Copy link
Member

Now that cargo rustc is implemented (and this is part of custom target specs), I'm going to close this as I think the need is probably less pressing for now. I'd still like to continue to avoid exposing all flags of the compiler through Cargo itself, and cargo rustc should be a good escape hatch for this behavior for now.

@huonw
Copy link
Member Author

huonw commented Jun 3, 2015

Hm, it was pointed out to me that cargo rustc doesn't quite handle everything. Specifically, AFAIK, it doesn't allow one to easily build dependencies with the given codegen options, which can be pretty important.

(I could be wrong, but reopening so this doesn't get forgotten. Close if I am.)

@huonw huonw reopened this Jun 3, 2015
@alexcrichton
Copy link
Member

Yeah cargo rustc is not appropriate for the use case of "I'd like to apply this set of LLVM features to the entire compile", but I also don't think that exposing these flags is the right option. The "correct" way to do that is to create a custom target specification which has the right set of features listed, depending on your use case.

Can you elaborate on what the use case is for this? I would expect that a one-off compile (testing using new instructions) would be done through cargo rustc, but do you want this saved into Cargo.toml, for example? (without switching targets fundamentally).

@huonw
Copy link
Member Author

huonw commented Jun 3, 2015

"Make this binary run as fast as possible here" essentially implies feeding -C target-cpu=native to each crate. I suppose it could be handled via target specs, but that seems... way more difficult than necessary. Maybe a built-in target like cargo build --target=native.

However, that doesn't cover cases like "I'm building for other computers where every CPU is guaranteed to support SSE4.1 (but not necessarily anything later)". It does seem like a custom target may work better for this: e.g. my-data-center-target.json.

Another case I can imagine is running tests/benchmarks with various cpu-... options, but this is maybe less important?

@alexcrichton
Copy link
Member

Hm yeah I think those are compelling enough to not be totally covered by cargo rustc + custom target specs. I suspect that "run as fast as possible" typically doesn't require that all crates are compiled with target-cpu=native but rather just one or two particular ones, but the point definitely still stands.

Before we expose something like this I just want to make sure that we have a clear policy on what is/isn't exposed. I don't want all compiler options to become Cargo options in 3 different places (taken to the extreme).

@alexcrichton
Copy link
Member

With support for RUSTFLAGS now landed, I'm going to close this in favor of that as I believe it's possible to pass through that vector.

@kornelski
Copy link
Contributor

kornelski commented Jul 3, 2017

RUSTFLAGS is a very unsatisfactory solution.

  1. set RUSTFLAGS= is a shared mutable state. It's easy to forget to set the flags, and easy to forget to change the flags when switching between projects.

  2. Syntax for setting env vars is OS-dependent. It leads to silly cases where a Windows-only setting is documented using a Unix-specific syntax that doesn't work in Windows command prompt:

    RUSTFLAGS='-C target-feature=+crt-static' cargo build --target x86_64-pc-windows-msvc
    

    'RUSTFLAGS' is not recognized as an internal or external command,
    operable program or batch file.

  3. RUSTFLAGS is separate from Cargo.toml, so once a project depends on the flags, it is no longer a pure self-documenting Cargo project. The flags need to be documented or set using a custom (non-Cargo) build script.

    What I admire about Rust/Cargo is that complex projects can be built without RTFM, with just a plain cargo build. Dependency on RUSTFLAGS breaks that, which is a huge loss for me.

  4. Accidentally running cargo build without appropriate RUSTFLAGS may appear to work, but build subtly wrong things, e.g. a Windows exe that will work on my machine, but won't work on other people's machines.

  5. It's not suitable for cross-compilation, because during the build you may need different flags for native build.rs and cross-compiled products.

@Lokathor
Copy link

Lokathor commented Feb 6, 2018

I'd like to echo everything said in the above comment.

@kornelski
Copy link
Contributor

Exactly as predicted, I forgot to set RUSTFLAGS and distributed a "broken" executable to my users.

Please, please, please, allow target-feature to be set permanently in Cargo.toml!

@retep998
Copy link
Member

@kornelski For what it's worth, you can at least set rustflags in your .cargo/config on a per target basis. You can even put that .cargo/config in your project folder and check it in to git (although it won't have any effect when someone merely depends on your crate).

@awakecoding
Copy link

+1 on this request. It's really important to be able to control this more easily than with an environment variable.

@Dantali0n
Copy link

I would also greatly appreciate if target-feature could be set in Cargo.toml

For example, it would allow to more easily measure the performance impact of retpoline on specific models of CPU's.

bors added a commit that referenced this issue Apr 7, 2020
Upgrade to mdBook v0.3.7

This bumps the requirement from Rust v1.34.0 to v1.35.0 for building docs. AFAICT CI is using nightlies so that should be fine, but I thought I'd mention it in case someone thinks this impacts contributors in any way.

Other than that, there are a few changes that might impact some users in a visible way, like automatic dark theme support for those who picked that perference in their browser, possible color changes to the scrollbar and to the font size, change in the spacing in the sidebar entries, and many more changes and fixes that won't be too immediately impactful but very good all around.

I checked changes from transitive dependency bumps as well, AFAICT there is nothing that *should* impact the final rendering.

**tl;dr:** Nothing will explode. Probably.

For completeness, my raw notes of outtakes as I was reviewing the change logs:

```
[cosmetic]
v0.3.2 https://github.com/rust-lang/mdBook/blob/master/CHANGELOG.md#mdbook-032
Added automatic dark-theme detection based on the CSS prefers-color-scheme feature. This may be enabled by setting output.html.preferred-dark-theme to your preferred dark theme. #1037
rust-lang/mdBook#1037

v0.3.3 https://github.com/rust-lang/mdBook/blob/master/CHANGELOG.md#mdbook-033
Improvements to the automatic dark theme selection. #1069
rust-lang/mdBook#1069

v0.3.7 https://github.com/rust-lang/mdBook/blob/master/CHANGELOG.md#mdbook-037
Fixed theme selector focus. #1170
rust-lang/mdBook#1170

* https://developer.mozilla.org/en-US/docs/Web/CSS/@media/prefers-color-scheme
* users who picked the dark color scheme in their browser will see the cargo doc in dark.

[cosmetic]
v0.3.2 https://github.com/rust-lang/mdBook/blob/master/CHANGELOG.md#mdbook-032
Use standard scrollbar-color CSS along with webkit extension #816
rust-lang/mdBook#816

* https://developer.mozilla.org/en-US/docs/Web/CSS/scrollbar-color
* scroll bar color might change i guess.

[helpful]
v0.3.2 https://github.com/rust-lang/mdBook/blob/master/CHANGELOG.md#mdbook-032
Updated highlight.js for syntax highlighting updates (primarily to add async/await to Rust highlighting). #1041
rust-lang/mdBook#1041

* not sure cargo doc has many code examples with async/await, but there we go.

[warning]
v0.3.2 https://github.com/rust-lang/mdBook/blob/master/CHANGELOG.md#mdbook-032
Raised minimum supported rust version to 1.35. #1003
rust-lang/mdBook#1003

* from 1.34.0.

[cosmetic]
v0.3.4 https://github.com/rust-lang/mdBook/blob/master/CHANGELOG.md#mdbook-034
Switch to relative rem font sizes from px. #894
rust-lang/mdBook#894

* will impact some displays, but px is already an abstract thing so maybe not that big of an impact.

[warning]
v0.3.5 https://github.com/rust-lang/mdBook/blob/master/CHANGELOG.md#mdbook-035
Updated pulldown-cmark to 0.6.1, fixing several issues. #1021
rust-lang/mdBook#1021

* from 0.5, breaking changes.
* parsing only -- the team had to do multiple changes but nothing seems like it would impact final rendering

[cosmetic]
v0.3.6 https://github.com/rust-lang/mdBook/blob/master/CHANGELOG.md#mdbook-036
Adjusted spacing of sidebar entries. #1137
rust-lang/mdBook#1137

[warning]
v0.3.6 https://github.com/rust-lang/mdBook/blob/master/CHANGELOG.md#mdbook-036
Handlebars updated to 3.0. #1130
rust-lang/mdBook#1130

* from 2.0
* https://github.com/sunng87/handlebars-rust/blob/master/CHANGELOG.md
* strictly maintenance and perf AFAICS, no changes to final rendering.

[cosmetic]
v0.3.6 https://github.com/rust-lang/mdBook/blob/master/CHANGELOG.md#mdbook-036
Adjusted the menu bar animation to not immediately obscure the top content. #989
rust-lang/mdBook#989

* personal fave.

[cosmetic]
v0.3.7 https://github.com/rust-lang/mdBook/blob/master/CHANGELOG.md#mdbook-037
Code spans in headers are no longer highlighted as code. #1162

* users will see some headers change, probably.

[fixes]
+ ~13 fixes impacting rendering in less immediately visible ways.
rest should have no impact on end-user experience.
```
@moonheart08
Copy link

Can we please get this in. This would be unimaginably helpful.

@dagronlund
Copy link

Has this been addressed? I want to continue echoing all the above comments on why this is an important cargo feature to have.

@Corfucinas
Copy link

@kornelski For what it's worth, you can at least set rustflags in your .cargo/config on a per target basis. You can even put that .cargo/config in your project folder and check it in to git (although it won't have any effect when someone merely depends on your crate).

Just doing this fixed my problems (monorepo at work with a team of 10, so now new members won't need to modify ~/.cargo/config.toml

@recatek
Copy link

recatek commented Jul 9, 2022

Also very interested in some version of this. The workaround of setting rustflags in .cargo/config.toml is error-prone and complicates redistribution for hardware-specific build optimizations. It also can be easily lost or overlooked in large workspaces that are used to looking solely at Cargo.toml for build-specific information.

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

No branches or pull requests