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

Update codegen option documentation. #65136

Merged
merged 4 commits into from
Nov 5, 2019

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Oct 5, 2019

Some documentation updates:

  • Add more detail to codegen options.
  • Add missing options:
    • force-frame-pointers
    • default-linker-libraries
    • linker-plugin-lto
  • Add fragment anchors for all command-line-arguments.
  • Add some cross links between options.

@rust-highfive
Copy link
Collaborator

r? @GuillaumeGomez

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 5, 2019
@JohnCSimon
Copy link
Member

Ping from triage
@GuillaumeGomez can you please review this?
Thanks

@bors
Copy link
Contributor

bors commented Oct 18, 2019

☔ The latest upstream changes (presumably #64925) made this pull request unmergeable. Please resolve the merge conflicts.

@ehuss ehuss force-pushed the update-codegen-options branch from 5544008 to e0559aa Compare October 18, 2019 17:45
@Dylan-DPC-zz
Copy link

r? @Dylan-DPC

@bors
Copy link
Contributor

bors commented Oct 23, 2019

☔ The latest upstream changes (presumably #65716) made this pull request unmergeable. Please resolve the merge conflicts.

@ehuss ehuss force-pushed the update-codegen-options branch from e0559aa to 4b04440 Compare October 23, 2019 14:16
@JohnCSimon
Copy link
Member

Hello from triage
@Dylan-DPC can you please review this PR?
cc: @ehuss ehuss
Thanks

Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Dylan-DPC I've left my comments in-line.

src/doc/rustc/src/codegen-options/index.md Show resolved Hide resolved
src/doc/rustc/src/codegen-options/index.md Outdated Show resolved Hide resolved
src/doc/rustc/src/codegen-options/index.md Outdated Show resolved Hide resolved
src/doc/rustc/src/codegen-options/index.md Outdated Show resolved Hide resolved
src/doc/rustc/src/codegen-options/index.md Outdated Show resolved Hide resolved
src/doc/rustc/src/codegen-options/index.md Outdated Show resolved Hide resolved

The default is 225.
The default depends on the [opt-level](#opt-level). Current values are between
25 to 275.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given this, should we indicate the scores on the optimisation level table above, or is this too subject to change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm often reluctant to go into details on how optimizations work. The correct settings usually require experimentation, and heavily depend on the project. They also are very LLVM specific, and are complex. However, I can see how this is annoyingly vague. I added a table, it can be updated or removed in the future if it ever changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, this can be resolved then.

@ehuss ehuss force-pushed the update-codegen-options branch from e5dbff4 to c6bfe28 Compare October 26, 2019 16:06
Copy link
Contributor Author

@ehuss ehuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for taking a look!


The default is 225.
The default depends on the [opt-level](#opt-level). Current values are between
25 to 275.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm often reluctant to go into details on how optimizations work. The correct settings usually require experimentation, and heavily depend on the project. They also are very LLVM specific, and are complex. However, I can see how this is annoyingly vague. I added a table, it can be updated or removed in the future if it ever changes.

Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this looks good.

@JohnCSimon
Copy link
Member

Ping from triage -
@Dylan-DPC Seems @ehuss has addressed the issues
That can you please review this PR?

Thanks

@Dylan-DPC-zz
Copy link

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Nov 2, 2019

📌 Commit c6bfe28 has been approved by Dylan-DPC

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 2, 2019
Comment on lines 79 to 80
units](#codegen-units). In this case, LTO is disabled if codegen units is 1 or
optimizations are disabled ([`-C opt-level=0`](#opt-level)).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this true?
I tried to set exclusive lto and codegen-units option and the binary size is
increased than when I set both lto=true and codegen-units=1.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By "exclusive LTO", do you mean not including the flag? Can you clarify which commands you ran?

I would expect it to be larger, since lto=true is "fat", and without the flag it is only "thin local", which does less optimization.

Here is the logic this is referring to.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By "exclusive" I mean I either set lto or codegen-units.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I set these options in Cargo.toml, not via rustc arguments

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not sure I understand the confusion here. lto=true + codegen-units=1 should be the smallest. It does fat LTO, and with 1 CGU per crate, so it has more optimization potential (but is very slow to compile). That is:

  • codegen-units=1: Disables LTO, so won't have cross-crate optimizations.
  • lto=true: 16 cgus, performs fat LTO across crates.
  • codegen-units=1 + lto=true: 1 cgu, fat LTO across crates.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO adding that to the documentation seems really helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll try to make it a little clearer.

@ehuss
Copy link
Contributor Author

ehuss commented Nov 2, 2019

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 2, 2019
@ehuss
Copy link
Contributor Author

ehuss commented Nov 2, 2019

@lzutao @Dylan-DPC can you take a look at the last commit and see if it helps? Not sure if that really clarifies it not.

Comment on lines +77 to +81
If `-C lto` is not specified, then the compiler will attempt to perform "thin
local LTO" which performs "thin" LTO on the local crate only across its
[codegen units](#codegen-units). When `-C lto` is not specified, LTO is
disabled if codegen units is 1 or optimizations are disabled ([`-C
opt-level=0`](#opt-level)). That is:
Copy link
Contributor

@tesuji tesuji Nov 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this?

[codegen-units] and [opt-level] also affects on how compiler perform LTO:
* When `-C lto` is not specified:
  - `codegen-units=1`: Disables LTO.
  - `opt-level=0`: Disables LTO.
* When `-C lto=true`:
  - `codegen-units=1`: perform fat LTO across crates with 1 codegen unit. Some people prefers to do it in cargo release profile.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, updated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. That's clearer.

@ehuss ehuss force-pushed the update-codegen-options branch from 3538ffb to 9b9d651 Compare November 3, 2019 14:01
@Dylan-DPC-zz
Copy link

@lzutao ping me when you think this is ready to be merged

@tesuji
Copy link
Contributor

tesuji commented Nov 3, 2019

@Dylan-DPC My pleasure. I think this PR is ready to merge.

@Dylan-DPC-zz
Copy link

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Nov 4, 2019

📌 Commit 9b9d651 has been approved by Dylan-DPC

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 4, 2019
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Nov 5, 2019
…an-DPC

Update codegen option documentation.

Some documentation updates:

- Add more detail to codegen options.
- Add missing options:
    - `force-frame-pointers`
    - `default-linker-libraries`
    - `linker-plugin-lto`
- Add fragment anchors for all command-line-arguments.
- Add some cross links between options.
bors added a commit that referenced this pull request Nov 5, 2019
Rollup of 10 pull requests

Successful merges:

 - #65136 (Update codegen option documentation.)
 - #65574 (docs: improve disclaimer regarding LinkedList)
 - #65720 (Add FFI bindings for LLVM's Module::getInstructionCount())
 - #65905 ([doc] fixes for unix/vxworks `OpenOptionsExt::mode`)
 - #65962 (Fix logic in example.)
 - #66019 (Improved std::iter::Chain documentation)
 - #66038 (doc(str): show example of chars().count() under len())
 - #66042 (Suggest correct code when encountering an incorrect trait bound referencing the current trait)
 - #66073 (Do not needlessly write-lock)
 - #66096 (Add a failing UI test for multiple loops of all kinds in a `const`)

Failed merges:

r? @ghost
@bors bors merged commit 9b9d651 into rust-lang:master Nov 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants