Uh oh!
There was an error while loading. Please reload this page.
Fix constant propagation for scalar pairs - #67015
Merged
Merged
Conversation
rust-highfive
commented
Dec 4, 2019
Contributor
r? @davidtwco (rust_highfive has picked a reviewer for you, use r? to override) |
osa1
commented
Dec 4, 2019
ContributorAuthor
(fixed a long line) |
tmiasko
reviewed
Dec 4, 2019
Contributor
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 betweenScalar and ScalarPair layouts, for example in something like test(((1, 2),)),
when propagating the value of the outermost tuple.
ContributorAuthor
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.
Centril
commented
Dec 4, 2019
Contributor
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
wesleywiser
commented
Dec 4, 2019
Member
Can you also check if this fixes #66339? |
osa1
commented
Dec 4, 2019
ContributorAuthor
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). |
Member
We definitely should not be doing that. |
osa1
commented
Dec 4, 2019
ContributorAuthor
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? |
wesleywiser
commented
Dec 4, 2019
Member
@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
Uh oh!
There was an error while loading. Please reload this page.
oli-obk
commented
Dec 6, 2019
Contributor
r? @wesleywiser |
wesleywiser
commented
Dec 6, 2019
Member
The build is failing due to tidy:
|
rust-highfive
commented
Dec 6, 2019
Contributor
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 |
osa1
commented
Dec 6, 2019
ContributorAuthor
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? |
oli-obk
commented
Dec 6, 2019
Contributor
Well... in that case you can just move the tests to |
osa1
commented
Dec 6, 2019
ContributorAuthor
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. |
osa1
commented
Dec 6, 2019
ContributorAuthor
Let's add the MIRs. I'll update. |
oli-obk
commented
Dec 6, 2019
Contributor
Including more MIR tests indeed is a good idea, we don't have enough of these tests and random stuff keeps changing there |
wesleywiser
commented
Dec 6, 2019
Member
I personally would lean toward including the MIR because then when you open that other PR, it's a lot easier to see the improvement because the tests will change. |
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#66971Fixes#66339Fixes#67019
bors
commented
Dec 9, 2019
Collaborator
⌛ Testing commit 2404a06 with merge 59264e56504466ca5619b0918148e828c6aa61c9... |
rust-highfive
commented
Dec 9, 2019
Contributor
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
commented
Dec 9, 2019
Collaborator
💔 Test failed - checks-azure |
wesleywiser
commented
Dec 9, 2019
Member
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
|
oli-obk
commented
Dec 10, 2019
Contributor
@bors retry This PR makes an optimization less aggressive, I don't see how it could be causing this new failure |
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. Fixesrust-lang#66971Fixesrust-lang#66339Fixesrust-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
osa1
commented
Dec 11, 2019
ContributorAuthor
So that was an intermittent (non-deterministic?) test failure? I guess every project has them 😅 |
Contributor
Did we never run perf on this? My guess is that this regression was caused by this PR. (EDIT: probably not, see below) |
wesleywiser
commented
Dec 12, 2019
Member
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 Fixesrust-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 Fixesrust-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)
This file contains hidden or 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
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
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