Repository navigation
de-stabilize optimize(speed) - #165592
de-stabilize optimize(speed)#165592RalfJung wants to merge 1 commit into
Conversation
|
Some changes occurred in compiler/rustc_attr_parsing |
|
r? @mu001999 rustbot has assigned @mu001999. Use Why was this reviewer chosen?The reviewer was selected based on:
|
dc76ebb to
1990c23
Compare
|
If I understand correctly that code, it does do something: it omits |
|
Fair, I updated the description. |
|
I'm OK either way on this, but I'll ask: is this worth reverting? It's just a hint. We're allowed to ignore it. I feel as though we do know what this is supposed to do.1 To the degree that it's not yet doing that, it seems something we could fix post-stabilization. Footnotes
|
To elaborate on this: to some extent it's fine and expected that
|
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
Do I understand correctly that you want to destabilize At So I don't think this PR makes sense. |
So, what exactly is it supposed to do that it's not doing today? If there's a conflict between what "we know" it's supposed to do vs. how it actually works, then that should give us pause and prompt us to reconsider. After all, there are good reasons why LLVM works the way it does. Of course the attribute is a hint that is allowed to do nothing, but we do want it to do something that is both useful and actually implementable. So, for example, if people "know" that putting
Per the recent Zulip discussion, the difference is in expectations: some people would like |
1990c23 to
41ab21c
Compare
Yes, we are, but I don't see who we are helping by having an attribute that basically never does what people expect it to do.
No, I want to destabilize it because it basically never works.
|
Following @nikic's logic above about symmetry,1 my feeling is that either we should be leaning into the idea that these attributes are just hints, or we should be reverting the entire stabilization. Footnotes
|
|
I think that's throwing out the baby with the bathwater.
I thought we'd not have all the right passes in the pipeline for optimize(speed) to do anything useful there? Also FWIW on mir-opts the behavior is quite different from what you describe. |
FWIW I do have projects where “release build” means opt-level=s, and But since the implications of this for optimize(speed) are surprising to several people and apparently weren’t fully understood by everyone during the FCP, I guess it does make sense to go back and try to get everyone on the same page about what optimize(speed) actually does. Probably we don’t need to actually agree on which behavior would be most desirable in theory, and probably we should stabilize the actual behavior anyway sooner rather than later, but it’s easier to have the conversation without the time pressure of the stabilization being in beta already. |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
We do have all the opt-level=2 passes (which is almost all of them) in the pipeline at opt-level=s. Although as I mentioned before, they may have other knobs besides the one encoded in Not sure if that's a problem in LLVM itself (and if so, whether it's already fixed in main) or due to one of the places in rustc that branches on opt-level. #155656 only updated the mapping to LLVM's enum, but I recall that we do a lot more decisions in configuring the pipeline. |
|
Oh yeah we do tell LLVM to disable some passes including loop unrolling and vectorization based on rustc's own |
Ah I see, I was not aware of that.
Hm, that does make sense. I would however argue it then also makes sense to do something similar for MIR opts, i.e. a |
Yeah that would be more consistent. I don't think there's currently much harm in running more mir-opt passes, if it's easy to support and doesn't cost much compile time (especially not for the vast majority of code not annotated |
I reported this as #165601. It's not a fundamental problem with what the attribute means, but as long as we have that bug, The fact that this wasn't noticed before indicates to me that nobody actually used this attribute in anger yet, which is maybe a bad sign for stabilizing it. |
I think we should de-complicate the code so that the inliner runs.^^ The current logic is strange. |
View all comments
Reverts parts of #157273.
We don't actually know what this attribute should do (see Zulip). In LLVM, the opt-level is used to configure the entire pass pipeline, it is not possible to overwrite this for a single function. Correspondingly, it currently does very little: it suppresses a global
-Copt-level=s/z, but that's it.Cc @rust-lang/lang
Tracking issue: #165590