Fix constant propagation for scalar pairs #67015
Merged
+130
−11
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? @davidtwco (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
(fixed a long line) |
tmiasko
reviewed
Dec 4, 2019
| let scalar = if field_layout.details.size == Size::ZERO { | ||
| Scalar::zst() | ||
| } else { | ||
| pair_values.pop().unwrap() |
There was a problem hiding this comment.
I think there might be a need for an additional check that the field actually
has Abi::Scalar layout. Otherwise it could end up with an assignment between
Scalar and ScalarPair layouts, for example in something like test(((1, 2),)),
when propagating the value of the outermost tuple.
There was a problem hiding this comment.
Amazing, your example uncovers another bug. I reported it as #67019.
I'll try to fix it in this PR too.
|
Can you also check if this fixes #66339? |
It does not, but it changes the error. For actually fixing these both I think it'd be good to first decide whether we should be changing tuple reprs in const-prop (hence the Zulip topic). |
We definitely should not be doing that. |
|
OK, in that case I think this PR will change quite a bit and the final PR should fix both of these issues. I'm running out of time for today but I'll try to update sometime later this week. I hope this is not blocking anyone? |
|
@osa1 Sorry, my point was supposed to be that I think your PR is overall the right direction to go in. I think there's probably a few small tweaks you should make per the above feedback and then we can land this. |
oli-obk
reviewed
Dec 4, 2019
osa1
changed the title
|
r? @wesleywiser |
|
The build is failing due to tidy:
|
|
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 |
I'd be happy with either but I wonder if we should be including MIRs just yet because as far as I understand from the Zulip discussion we want to optimize scalar pairs more aggressively in the future so MIRs for these tests are currently not final. Same goes for the test for #66971 too actually. Opinions? |
|
Well... in that case you can just move the tests to |
|
Yes, that's what @wesleywiser suggested also. I'm just asking whether I should do that, or include current MIRs and keep the tests in current location. |
|
Let's add the MIRs. I'll update. |
|
⌛ Testing commit 2404a06 with merge 59264e56504466ca5619b0918148e828c6aa61c9... |
|
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 |
|
💔 Test failed - checks-azure |
bors
added
S-waiting-on-review
|
I'm completely unfamiliar with this part of the CI process. It looks like we're testing various crates from the ecosystem and one of the
|
|
@bors retry This PR makes an optimization less aggressive, I don't see how it could be causing this new failure |
bors
added
S-waiting-on-bors
Centril
added a commit
to Centril/rust
that referenced
this pull request
Dec 11, 2019
Fix constant propagation for scalar pairs We now only propagate a scalar pair if the Rvalue is a tuple with two scalars. This for example avoids propagating a (u8, u8) value when Rvalue has type `((), u8, u8)` (see the regression test). While this is a correct thing to do, implementation is tricky and will be done later. Fixes rust-lang#66971 Fixes rust-lang#66339 Fixes rust-lang#67019
bors
added a commit
that referenced
this pull request
Dec 11, 2019
Rollup of 6 pull requests Successful merges: - #66881 (Optimize Ord trait implementation for bool) - #67015 (Fix constant propagation for scalar pairs) - #67074 (Add options to --extern flag.) - #67164 (Ensure that panicking in constants eventually errors) - #67174 (Remove `checked_add` in `Layout::repeat`) - #67205 (Make `publish_toolstate.sh` executable) Failed merges: r? @ghost
|
So that was an intermittent (non-deterministic?) test failure? I guess every project has them 😅 |
|
Did we never run perf on this? My guess is that this regression was caused by this PR. (EDIT: probably not, see below) |
|
We didn't but I'm not sure this PR is responsible because there are so many check regressions. When we turned on const prop by default, we only really saw improvements on debug & release builds not check builds because check builds don't query for optimized mir. |
Centril
added a commit
to Centril/rust
that referenced
this pull request
Dec 13, 2019
… r=oli-obk Const prop should finish propagation into user defined variables Fixes rust-lang#66638 ~~Temporarily rebased on top of rust-lang#67015 to get those fixes.~~ r? @oli-obk
Centril
added a commit
to Centril/rust
that referenced
this pull request
Dec 20, 2019
… r=oli-obk Const prop should finish propagation into user defined variables Fixes rust-lang#66638 ~~Temporarily rebased on top of rust-lang#67015 to get those fixes.~~ r? @oli-obk
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jul 20, 2023
Always const-prop scalars and scalar pairs This removes some complexity from the pass. The limitation to propagate ScalarPairs only for tuple comes from rust-lang#67015, when ScalarPair constant were modeled using `Rvalue::Aggregate`. Nowadays, we use `ConstValue::ByRef`, which does not care about the underlying type. The justification for not propagating in all cases was perf. This seems not to be a clear cut any more: rust-lang#113858 (comment)
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.
We now only propagate a scalar pair if the Rvalue is a tuple with two scalars. This for example avoids propagating a (u8, u8) value when Rvalue has type
((), u8, u8)(see the regression test). While this is a correct thing to do, implementation is tricky and will be done later.Fixes #66971
Fixes #66339
Fixes #67019