Implement optimize(size) and optimize(speed) attributes #55641
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
cc #54882 |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
nikic
reviewed
Nov 3, 2018
src/librustc_codegen_llvm/base.rs
Outdated
| // If any of the items require optimize(speed), we must use speed optimisation globally | ||
| // and set either `optnone` or corresponding size-oriented optimisation level for the | ||
| // rest of the items in the crate. Those attributes are set specifically | ||
| for_speed |
There was a problem hiding this comment.
I suspect that this will have some unintended fallout. While function passes are going to skip optnone functions, we'll also end up populating a full module pass manager, where likely not all module passes will ignore optnone functions (not to mention non-functions -- I don't believe there is an optnone for global variables.) Marking a single function for optimize(speed) may end up influencing codegen for the whole crate in non-trivial ways.
There was a problem hiding this comment.
This is a known caveat. Alas, to get optimize(speed) (or optimize(speed), now that I think about it) to work as intended, it is necessary to initialize some sort of pass manager.
Are you saying it is possible to have separate managers for functions and the rest of the module? That might be a better approach then.
There was a problem hiding this comment.
Alternative is to have these attributes not work with -Copt-level=0 at all, but that is not what I intended when I wrote the RFC.
There was a problem hiding this comment.
Are you saying it is possible to have separate managers for functions and the rest of the module? That might be a better approach then.
As far as I'm aware that's not possible. While LLVM has a separate function pass manager, it is only used for a few early optimizations, the bulk of the work (including most function passes) are performed by the module pass manager.
Alternative is to have these attributes to not work with -Copt-level=0 at all, but that is not what I intended when I wrote the RFC.
This would probably be the most predictable approach. It would make optimize(size) control the optsize attribute and optimize(speed) an opt-out for the optsize attribute (which, having looked at the RFC thread now, appears to be the motivation for having it). Either attribute would have no impact on global codegen, which is still controlled by -C opt-level. -C opt-level=0 would continue to mean "don't touch my code".
levex
reviewed
Nov 3, 2018
|
☔ The latest upstream changes (presumably #55349) made this pull request unmergeable. Please resolve the merge conflicts. |
|
(What's here so far seems fine to me. Of course its WIP and it sounds like there may be future changes that will also need review.) |
pnkfelix
added
S-waiting-on-author
|
The weakness of |
|
Ping from triage @nagisa: What is the status of this PR? |
|
I’ll get back to this as soon as I get the other tasks off my plate and time onto it :) |
nagisa
force-pushed
the
optimize-attr
branch
from
9201f8a
to
b45e5ac
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
nagisa
force-pushed
the
optimize-attr
branch
from
61526d3
to
7701073
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
nagisa
force-pushed
the
optimize-attr
branch
from
7701073
to
b38143d
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The failing test seems to be invalid. Namely, it seems that the test is compiling with optimisations disabled and yet checking for cc @alexcrichton do you know/remember why the test is compiled without optimisations? Will the test be just as valid if I slap a |
|
I think that's a test that |
|
Yes, as of this PR inline(always) will not end up in an equivalent
attribute in LLVM if it would otherwise also add an optnone (happens with
-Copt-level=0). This is because llvm verification requires noinline to be
present along with optnone.
When I said this test was invalid, I was meanin to point out that this test
does not test what it possibly could be intended to test. None of the
inline attributes are relevant in opt-level=0 as we dont even initialize
the pass manager (to care about tuos attribute) at that level.
So what I'm trying to figure out is what the intention was with this test.
I could just make it run with some light optimisations and perhaps even
(-Cno-prepopulate-passes) and it would continue passing fine and most
likely testing exactly what the test was intending to test.
…On Mon, Nov 26, 2018, 05:48 Alex Crichton ***@***.*** wrote:
I think that's a test that #[inline(always)] is respected in all modes,
including opt-level=0. Did this PR break that though?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#55641 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AApc0jtA19ZGNYurRi5Q96W3viTObSC3ks5uy2SZgaJpZM4YMt2U>
.
|
|
We well the intention of the test is that we do respect the inline always attribute at O0, and we do execute the necessary passes to handle inline always functions. I don't believe this is functionality that we can break, crates rely on this in the wild |
|
Ah, okay, makes sense. I’m slightly surprised we run any passes at |
|
☔ The latest upstream changes (presumably #49219) made this pull request unmergeable. Please resolve the merge conflicts. |
nagisa
changed the title
|
I think this is ready-to-merge now. Perhaps it would have been a great idea to add support for revisions for codegen tests in a separate PR… but hey, it is now possible to have tests with their own optimisation levels! It might be sensible to open up revisions to other test types as well (which should be just removal of the panic/assertion away). @alexcrichton I used the new revisions support for codegen tests added in this PR to write a proper test for the |
|
Ping from triage @nagisa : Have you been able to make any progress on this? |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
☔ The latest upstream changes (presumably #57747) made this pull request unmergeable. Please resolve the merge conflicts. |
pnkfelix
added
S-waiting-on-review
pnkfelix
reviewed
Jan 24, 2019
src/librustc_codegen_llvm/declare.rs
Outdated
| @@ -65,15 +65,24 @@ fn declare_raw_fn( | |||
| } | |||
| } | |||
|
|
|||
| match cx.tcx.sess.opts.cg.opt_level.as_ref().map(String::as_ref) { | |||
| Some("s") => { | |||
| // FIXME(opt): this is kinda duplicated with similar code in attributes::from_fm_attrs… | |||
There was a problem hiding this comment.
these are in the same crate; can we lift it out to a common method? Or is there some difference I'm missing...
There was a problem hiding this comment.
(some things are different in what we call for e.g. Speed; cannot tell offhand if that is actually significant or not. So I guess this can stay as is for now.)
There was a problem hiding this comment.
I extracted the common code into a function.
|
okay r=me after a rebase. I trust @nagisa has a good reason for not lifting the match with the "kinda duplicated" out into a helper. |
Alas it does not currently work, because of limitations in compiletest…
`compile-flags: -Copt-level` will avoid adding -O. Similarly for -g and -Cdebuglevel.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@bors r=pnkfelix Finally? Go! Go! Go! |
|
📌 Commit ce289c6 has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Jan 26, 2019
Implement optimize(size) and optimize(speed) attributes This PR implements both `optimize(size)` and `optimize(speed)` attributes. While the functionality itself works fine now, this PR is not yet complete: the code might be messy in places and, most importantly, the compiletest must be improved with functionality to run tests with custom optimization levels. Otherwise the new attribute cannot be tested properly. Oh, and not all of the RFC is implemented – attribute propagation is not implemented for example. # TODO * [x] Improve compiletest so that tests can be written; * [x] Assign a proper error number (E9999 currently, no idea how to allocate a number properly); * [ ] Perhaps reduce the duplication in LLVM attribute assignment code…
|
☀️ Test successful - checks-travis, status-appveyor |
|
☀️ Test successful - checks-travis, status-appveyor |
This was referenced Jan 26, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements both
optimize(size)andoptimize(speed)attributes.While the functionality itself works fine now, this PR is not yet complete: the code might be messy in places and, most importantly, the compiletest must be improved with functionality to run tests with custom optimization levels. Otherwise the new attribute cannot be tested properly. Oh, and not all of the RFC is implemented – attribute propagation is not implemented for example.
TODO