Replace fragile erroneous const sys #70820
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
rust-highfive
added
the
S-waiting-on-review
spastorino
force-pushed
the
replace-fragile-erroneous-const-sys
branch
from
49b1b19
to
522a0e2
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 |
Centril
reviewed
Apr 5, 2020
src/librustc_codegen_ssa/mir/mod.rs
Outdated
| @@ -190,6 +191,18 @@ pub fn codegen_mir<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>( | |||
|
|
|||
| fx.per_local_var_debug_info = fx.compute_per_local_var_debug_info(); | |||
|
|
|||
| for const_ in &mir.uneval_consts { | |||
There was a problem hiding this comment.
Assuming this PR is addressing #67191, it seems like the way this PR is setup, then cargo check (--emit metadata) would not trigger an error here but it should. In other words, erroneous live constants should be caught in analysis before optimizations.
There was a problem hiding this comment.
yea, we need to go through this list in the collector, too, just like we're doing with in-MIR constants right now. This may get solved automatically by extending the mir visitors to go through the constants in the list via visit_const
There was a problem hiding this comment.
Added this here 2f8c3b5
There was a problem hiding this comment.
@oli-obk I don't follow how this was resolved. The code was added to monomorphization logic, but presumably that never runs if we don't reach codegen?
There was a problem hiding this comment.
I don't see how cargo check can trigger an error on these without monomorphization. If you look at the test that we're doing all these changes for (https://github.com/rust-lang/rust/blob/master/src/test/ui/consts/assoc_const_generic_impl.rs) you see that without monomorphization we can't possibly report anything. This is inherently a monomorphization time thing.
There was a problem hiding this comment.
this applies to all other lints reported during mir optimizations, too. Const prop lints for example aren't triggered reliably from cargo check. Only if the optimized_mir query is called for a function, will const prop lints actually get reported. We could add optimized_mir runs for all functions to the analysis query though. In that case we'd also do a best effort evaluation of all constants
There was a problem hiding this comment.
Note that I feel that running optimized_mir on all functions for analysis is an orthogonal discussion to this PR and we should probably move it to an issue if you think this is a useful thing to do at all (it will make cargo check take more time as optimizations are run)
There was a problem hiding this comment.
Well this is all very unfortunate... I had neglected associated constants. With them in mind, the options as I see them are:
-
Error differently for polymorphic and monomorphic constants, doing the former post-monomorphization, and the latter pre-monomorphization (inconsistent).
-
Error for both post-monomorphization. (I consider all post-monomorphization errors to be language design bugs.)
-
Error for neither and insert a trap in codegen, but @RalfJung would probably be even more unhappy about that.
-
Only insert a trap for polymorphic constants (inconsistent).
There was a problem hiding this comment.
We are doing 1. right now. Erroring post monomorphization for associated constants is what we do to keep our APIs sane. Otherwise we'd need integer range bounds and more just to be able to do simple addition of associated constants inside other associated constants.
I don't like 3 and 4 and am not even sure that 2. is implementable 🤣
What we could do at some point in the future is to create a lint that tells you about sites where post monomorphization errors could occur depending on the generic parameters that were given, but that would require symbolic execution of constants and some sort of solver (not necessarily SAT) for them. While that is something we need for more advanced const generics trickeries anyway, I think we can just stay with option 1 until we address post monomorphization errors in general.
There was a problem hiding this comment.
We are doing 1. right now. Erroring post monomorphization for associated constants is what we do to keep our APIs sane. Otherwise we'd need integer range bounds and more just to be able to do simple addition of associated constants inside other associated constants.
Yea, this is going to be ungreat for the language spec and inconsistent, but of the bad choices I suppose it's the least bad (😭), so do your thing. :)
Centril
reviewed
Apr 5, 2020
spastorino
force-pushed
the
replace-fragile-erroneous-const-sys
branch
from
522a0e2
to
b98aedd
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 |
oli-obk
reviewed
Apr 7, 2020
oli-obk
reviewed
Apr 7, 2020
spastorino
force-pushed
the
replace-fragile-erroneous-const-sys
branch
from
6b158c6
to
ca9a5fd
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 latest upstream changes (presumably #70755) made this pull request unmergeable. Please resolve the merge conflicts. |
bjorn3
reviewed
Apr 16, 2020
bjorn3
reviewed
Apr 16, 2020
spastorino
force-pushed
the
replace-fragile-erroneous-const-sys
branch
from
ca9a5fd
to
19262f0
Compare
spastorino
force-pushed
the
replace-fragile-erroneous-const-sys
branch
6 times, most recently
from
2fb6333
to
d90c341
Compare
|
Once working and accepted this should not be rolled up. @bors rollup=never |
|
Let's do a perf run just to make sure this doesn't have any negative effects (and hopefully some positive ones). @bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit d90c3418677a4cc4a36038700f6ef3e7fe37cb01 with merge b8d790ba8c6168fc4039fb772e27c8274a25f61c... |
|
☔ The latest upstream changes (presumably #71467) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
added
S-waiting-on-author
Avoid exercising unevaluated constants and go straight to visit the blocks.
spastorino
force-pushed
the
replace-fragile-erroneous-const-sys
branch
from
543d514
to
7bc45f6
Compare
|
Rebased again. @bors r=oli-obk |
|
📌 Commit 7bc45f6 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-azure |
|
This was a major perf win for |
|
Interesting. @oli-obk @spastorino Is that expected or does this indicate we are failing to do some work now that we should be doing? |
|
That's very curious. I wouldn't have thought this to have any impact on perf. We're not doing the optimizations yet that would have a positive perf impact, so idk what's going on here. I'm doing another review round now with this in mind |
|
Ok so I thought what's happening:
But then I wondered how this would reduce just the time Looking at https://github.com/rust-lang/rustc-perf/blob/master/collector/benchmarks/ctfe-stress-4/src/lib.rs I believe this is a resonable explanation. |
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.
Closes #67191
r? @oli-obk