-
Notifications
You must be signed in to change notification settings - Fork 69
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
Remove -Zinline-in-all-cgus #814
Comments
This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed. Concerns or objections to the proposal should be discussed on Zulip and formally registered here by adding a comment with the following syntax:
Concerns can be lifted with:
See documentation at https://forge.rust-lang.org cc @rust-lang/compiler |
@rustbot second |
@rustbot label -final-comment-period +major-change-accepted |
…, r=nnethercore Remove -Zinline-in-all-cgus and clean up tests/codegen-units/ Implementation of rust-lang/compiler-team#814 I've taken some liberties with cleaning up the CGU partitioning tests, because that's the only place this flag was used and also mattered. I've often fought a lot with the contents of `tests/codegen-units` and it has never been clear to me when a test failure indicates a problem with my changes as opposed to a test just needing to be manually blessed. Hopefully the combination of the new README, new comments, and using `-Zprint-mono-items=lazy` in the partitioning tests improves that. I've also deleted some of the `tests/run-make/sepcomp` tests. I think all the "sepcomp" tests have been obviated for years by better-designed (less flaky, clearer failures) test suites, but here I'm just deleting the ones I'm confident in.
…r=<try> Remove -Zinline-in-all-cgus and clean up tests/codegen-units/ Implementation of rust-lang/compiler-team#814 I've taken some liberties with cleaning up the CGU partitioning tests, because that's the only place this flag was used and also mattered. I've often fought a lot with the contents of `tests/codegen-units` and it has never been clear to me when a test failure indicates a problem with my changes as opposed to a test just needing to be manually blessed. Hopefully the combination of the new README, new comments, and using `-Zprint-mono-items=lazy` in the partitioning tests improves that. I've also deleted some of the `tests/run-make/sepcomp` tests. I think all the "sepcomp" tests have been obviated for years by better-designed (less flaky, clearer failures) test suites, but here I'm just deleting the ones I'm confident in. try-job: x86_64-gnu-nopt
…, r=nnethercote Remove -Zinline-in-all-cgus and clean up tests/codegen-units/ Implementation of rust-lang/compiler-team#814 I've taken some liberties with cleaning up the CGU partitioning tests, because that's the only place this flag was used and also mattered. I've often fought a lot with the contents of `tests/codegen-units` and it has never been clear to me when a test failure indicates a problem with my changes as opposed to a test just needing to be manually blessed. Hopefully the combination of the new README, new comments, and using `-Zprint-mono-items=lazy` in the partitioning tests improves that. I've also deleted some of the `tests/run-make/sepcomp` tests. I think all the "sepcomp" tests have been obviated for years by better-designed (less flaky, clearer failures) test suites, but here I'm just deleting the ones I'm confident in.
…r=nnethercote Remove -Zinline-in-all-cgus and clean up tests/codegen-units/ Implementation of rust-lang/compiler-team#814 I've taken some liberties with cleaning up the CGU partitioning tests, because that's the only place this flag was used and also mattered. I've often fought a lot with the contents of `tests/codegen-units` and it has never been clear to me when a test failure indicates a problem with my changes as opposed to a test just needing to be manually blessed. Hopefully the combination of the new README, new comments, and using `-Zprint-mono-items=lazy` in the partitioning tests improves that. I've also deleted some of the `tests/run-make/sepcomp` tests. I think all the "sepcomp" tests have been obviated for years by better-designed (less flaky, clearer failures) test suites, but here I'm just deleting the ones I'm confident in.
…cote Remove -Zinline-in-all-cgus and clean up tests/codegen-units/ Implementation of rust-lang/compiler-team#814 I've taken some liberties with cleaning up the CGU partitioning tests, because that's the only place this flag was used and also mattered. I've often fought a lot with the contents of `tests/codegen-units` and it has never been clear to me when a test failure indicates a problem with my changes as opposed to a test just needing to be manually blessed. Hopefully the combination of the new README, new comments, and using `-Zprint-mono-items=lazy` in the partitioning tests improves that. I've also deleted some of the `tests/run-make/sepcomp` tests. I think all the "sepcomp" tests have been obviated for years by better-designed (less flaky, clearer failures) test suites, but here I'm just deleting the ones I'm confident in.
…cote Remove -Zinline-in-all-cgus and clean up tests/codegen-units/ Implementation of rust-lang/compiler-team#814 I've taken some liberties with cleaning up the CGU partitioning tests, because that's the only place this flag was used and also mattered. I've often fought a lot with the contents of `tests/codegen-units` and it has never been clear to me when a test failure indicates a problem with my changes as opposed to a test just needing to be manually blessed. Hopefully the combination of the new README, new comments, and using `-Zprint-mono-items=lazy` in the partitioning tests improves that. I've also deleted some of the `tests/run-make/sepcomp` tests. I think all the "sepcomp" tests have been obviated for years by better-designed (less flaky, clearer failures) test suites, but here I'm just deleting the ones I'm confident in.
Proposal
The flag
-Zinline-in-all-cgus
was added in rust-lang/rust#45075. I think before that PR,#[inline]
caused what we today call LocalCopy codegen to occur at any optimization level. In the current compiler, we only do LocalCopy codegen when optimizations are enabled.-Zinline-in-all-cgus=no -Copt-level={1,2,3}
will produce an optimized build where#[inline]
functions get the LLVMinlinehint
attribute, but still get GloballyShared codegen.-Zinline-in-all-cgus=yes -Copt-level=0
will produce an unoptimized build where#[inline]
functions still get LocalCopy codegen, but the LLVM inliner isn't run so the extra copies are useless. I think both scenarios where the flag has any effect are undesirable, and the only useful behavior is the compiler's behavior when the flag is not used.I believe the flag was added as a just-in-case way to opt back into the previous behavior and provide users additional control over codegen. But based on some GitHub code search, there are literally no users of this flag on GitHub, and the one commented-out use that I could find was a user trying to add
-Zinline-in-all-cgus=yes
to-Copt-level=3
to get more optimizations.We should delete the flag because it has no value. I dislike how it complicates instantiation mode selection and is used so heavily in the codegen-units test suite, which has caused me a lot of grief trying to improve instantiation mode selection on real code and also keep the tests passing. Removing this flag would make our tests look more like real-world use of the compiler.
Mentors or Reviewers
Process
The main points of the Major Change Process are as follows:
@rustbot second
.-C flag
, then full team check-off is required.@rfcbot fcp merge
on either the MCP or the PR.You can read more about Major Change Proposals on forge.
Comments
This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.
The text was updated successfully, but these errors were encountered: