Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use valtrees as the type-system representation for constant values #96591

Merged
merged 11 commits into from
Jun 14, 2022

Conversation

b-naber
Copy link
Contributor

@b-naber b-naber commented Apr 30, 2022

This is not quite ready yet, there are still some problems with pretty printing and symbol mangling and deref_const seems to not work correctly in all cases.

Mainly opening now for a perf-run (which should be good to go, despite the still existing problems).

r? @oli-obk

cc @lcnr @RalfJung

@rust-highfive
Copy link
Collaborator

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 30, 2022
@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

  • These commits modify submodules.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 30, 2022
@rust-log-analyzer

This comment has been minimized.

@b-naber b-naber force-pushed the transition-to-valtrees-in-type-system branch from e2a8722 to 4637c6f Compare April 30, 2022 21:45
@rust-log-analyzer

This comment has been minimized.

@b-naber b-naber force-pushed the transition-to-valtrees-in-type-system branch from 4637c6f to b3cf607 Compare April 30, 2022 22:04
@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented May 1, 2022

Unfortunately we need rustdoc to compile for perf runs

@rust-log-analyzer

This comment has been minimized.

@b-naber b-naber force-pushed the transition-to-valtrees-in-type-system branch from d724521 to de5c83a Compare May 2, 2022 11:47
@rust-log-analyzer

This comment has been minimized.

@b-naber b-naber force-pushed the transition-to-valtrees-in-type-system branch from de5c83a to da9d683 Compare May 2, 2022 13:10
@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented May 2, 2022

Let's see if this works. Maybe run ./x.py check in future PRs that touch datastructures in librustc_middle, as I'm not sure what else is needed for try builds

@bors try

@bors
Copy link
Contributor

bors commented May 2, 2022

⌛ Trying commit da9d68309443e7dd3afb9e0096db0ee8e4f55aa9 with merge 5db5e4101a6e3a17679a4ac488690c36745b53b8...

@bors
Copy link
Contributor

bors commented May 2, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 2, 2022
@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented May 2, 2022

Let's see if this works

nope 😆 all tools need to build at least

@b-naber
Copy link
Contributor Author

b-naber commented May 2, 2022

^^

Should work now. x.py check works. And before the changes to clippy and cranelift I was able to build stage2 with incremental turned off.

@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented May 2, 2022

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 2, 2022
@bors
Copy link
Contributor

bors commented May 2, 2022

⌛ Trying commit 7add06c03661a556e0eed1810d34d5f7f5b46f84 with merge 2b7377c196716fe4ec48ba949e509046e307ee32...

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (1f34da9): comparison url.

Instruction count

  • Primary benchmarks: mixed results
  • Secondary benchmarks: mixed results
mean1 max count2
Regressions 😿
(primary)
0.4% 0.6% 54
Regressions 😿
(secondary)
3.1% 14.3% 29
Improvements 🎉
(primary)
-3.0% -5.9% 10
Improvements 🎉
(secondary)
-1.1% -1.6% 7
All 😿🎉 (primary) -0.2% -5.9% 64

Max RSS (memory usage)

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: 😿 relevant regression found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
2.2% 2.2% 1
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) N/A N/A 0

Cycles

Results
  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: 😿 relevant regressions found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
12.8% 16.5% 7
Improvements 🎉
(primary)
-3.8% -5.6% 6
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) -3.8% -5.6% 6

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

@oli-obk
Copy link
Contributor

oli-obk commented Jun 15, 2022

The primary regressions are most likely noise or due to additional query encodings in incremental.

The big regression is the CTFE stress test, which, considering this PR heavily modifies how constants are handled, is somewhat expected. We're stress testing the worst case path: evaluating a constant to a valtree and then turning it back into an evaluator value without ever really needing the valtree value.

@Mark-Simulacrum Mark-Simulacrum added the perf-regression-triaged The performance regression has been triaged. label Jun 21, 2022
celinval added a commit to celinval/kani-dev that referenced this pull request Jul 6, 2022
The updates required were related to the following changes:
  - Simplify memory ordering intrinsics
    - rust-lang/rust#97423
  - once cell renamings
    - rust-lang/rust#98165
  - Rename the ConstS::val field as kind
    - rust-lang/rust#97935
  - Remove the source archive functionality of ArchiveWriter
    - rust-lang/rust#98098
  - Use valtrees as the type-system representation for constant values
    - rust-lang/rust#96591
celinval added a commit to model-checking/kani that referenced this pull request Jul 6, 2022
* Update toolchain to 2022-07-05

The updates required were related to the following changes:
  - Simplify memory ordering intrinsics
    - rust-lang/rust#97423
  - once cell renamings
    - rust-lang/rust#98165
  - Rename the ConstS::val field as kind
    - rust-lang/rust#97935
  - Remove the source archive functionality of ArchiveWriter
    - rust-lang/rust#98098
  - Use valtrees as the type-system representation for constant values
    - rust-lang/rust#96591

* Codegen unimplemented for unsupported constant slices

See #1339 for more details.

* Fix copyright check

* Use codegen_option_span instead
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 20, 2022
…-item, r=lcnr

Do not resolve associated const when there is no provided value

Fixes rust-lang#98629, since now we just delay a bug when we're not able to evaluate a const item due to the value not actually being provided by anything. This means compilation proceeds forward to where the "missing item in impl" error is emitted.

----

The root issue here is that when we're looking for the defining `LeafDef` in `resolve_associated_item`, we end up getting the trait's AssocItem instead of the impl's AssocItem (which does not exist). This resolution "succeeds" even if the trait's item has no default value, and then since this item has no value to evaluate, it turns into a const eval error.

This root issue becomes problematic (as in rust-lang#98629) when this const eval error happens in wfcheck (for example, due to normalizing the param-env of something that references this const). Since this happens sooner than the check that an impl actually provides all of the items that a trait requires (which happens during later typecheck), we end up aborting compilation early with only this un-informative message.

I'm not exactly sure _why_ this bug arises due to rust-lang#96591 -- perhaps valtrees are evaluated more eagerly than in the old system?

r? `@oli-obk` or `@lcnr` since y'all are familiar with const eval and reviewed rust-lang#96591, though feel free to reassign.

This is a regression from stable to beta, so I would be open to considering this for beta backport. It seems correct to me, especially given the improvements in the other UI tests this PR touches, but may have some side-effects that I'm unaware of...?
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jul 21, 2022
…-item, r=lcnr

Do not resolve associated const when there is no provided value

Fixes rust-lang#98629, since now we just delay a bug when we're not able to evaluate a const item due to the value not actually being provided by anything. This means compilation proceeds forward to where the "missing item in impl" error is emitted.

----

The root issue here is that when we're looking for the defining `LeafDef` in `resolve_associated_item`, we end up getting the trait's AssocItem instead of the impl's AssocItem (which does not exist). This resolution "succeeds" even if the trait's item has no default value, and then since this item has no value to evaluate, it turns into a const eval error.

This root issue becomes problematic (as in rust-lang#98629) when this const eval error happens in wfcheck (for example, due to normalizing the param-env of something that references this const). Since this happens sooner than the check that an impl actually provides all of the items that a trait requires (which happens during later typecheck), we end up aborting compilation early with only this un-informative message.

I'm not exactly sure _why_ this bug arises due to rust-lang#96591 -- perhaps valtrees are evaluated more eagerly than in the old system?

r? ``@oli-obk`` or ``@lcnr`` since y'all are familiar with const eval and reviewed rust-lang#96591, though feel free to reassign.

This is a regression from stable to beta, so I would be open to considering this for beta backport. It seems correct to me, especially given the improvements in the other UI tests this PR touches, but may have some side-effects that I'm unaware of...?
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jul 21, 2022
…-item, r=lcnr

Do not resolve associated const when there is no provided value

Fixes rust-lang#98629, since now we just delay a bug when we're not able to evaluate a const item due to the value not actually being provided by anything. This means compilation proceeds forward to where the "missing item in impl" error is emitted.

----

The root issue here is that when we're looking for the defining `LeafDef` in `resolve_associated_item`, we end up getting the trait's AssocItem instead of the impl's AssocItem (which does not exist). This resolution "succeeds" even if the trait's item has no default value, and then since this item has no value to evaluate, it turns into a const eval error.

This root issue becomes problematic (as in rust-lang#98629) when this const eval error happens in wfcheck (for example, due to normalizing the param-env of something that references this const). Since this happens sooner than the check that an impl actually provides all of the items that a trait requires (which happens during later typecheck), we end up aborting compilation early with only this un-informative message.

I'm not exactly sure _why_ this bug arises due to rust-lang#96591 -- perhaps valtrees are evaluated more eagerly than in the old system?

r? ```@oli-obk``` or ```@lcnr``` since y'all are familiar with const eval and reviewed rust-lang#96591, though feel free to reassign.

This is a regression from stable to beta, so I would be open to considering this for beta backport. It seems correct to me, especially given the improvements in the other UI tests this PR touches, but may have some side-effects that I'm unaware of...?
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jul 21, 2022
…-item, r=lcnr

Do not resolve associated const when there is no provided value

Fixes rust-lang#98629, since now we just delay a bug when we're not able to evaluate a const item due to the value not actually being provided by anything. This means compilation proceeds forward to where the "missing item in impl" error is emitted.

----

The root issue here is that when we're looking for the defining `LeafDef` in `resolve_associated_item`, we end up getting the trait's AssocItem instead of the impl's AssocItem (which does not exist). This resolution "succeeds" even if the trait's item has no default value, and then since this item has no value to evaluate, it turns into a const eval error.

This root issue becomes problematic (as in rust-lang#98629) when this const eval error happens in wfcheck (for example, due to normalizing the param-env of something that references this const). Since this happens sooner than the check that an impl actually provides all of the items that a trait requires (which happens during later typecheck), we end up aborting compilation early with only this un-informative message.

I'm not exactly sure _why_ this bug arises due to rust-lang#96591 -- perhaps valtrees are evaluated more eagerly than in the old system?

r? ````@oli-obk```` or ````@lcnr```` since y'all are familiar with const eval and reviewed rust-lang#96591, though feel free to reassign.

This is a regression from stable to beta, so I would be open to considering this for beta backport. It seems correct to me, especially given the improvements in the other UI tests this PR touches, but may have some side-effects that I'm unaware of...?
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jul 22, 2022
…-item, r=lcnr

Do not resolve associated const when there is no provided value

Fixes rust-lang#98629, since now we just delay a bug when we're not able to evaluate a const item due to the value not actually being provided by anything. This means compilation proceeds forward to where the "missing item in impl" error is emitted.

----

The root issue here is that when we're looking for the defining `LeafDef` in `resolve_associated_item`, we end up getting the trait's AssocItem instead of the impl's AssocItem (which does not exist). This resolution "succeeds" even if the trait's item has no default value, and then since this item has no value to evaluate, it turns into a const eval error.

This root issue becomes problematic (as in rust-lang#98629) when this const eval error happens in wfcheck (for example, due to normalizing the param-env of something that references this const). Since this happens sooner than the check that an impl actually provides all of the items that a trait requires (which happens during later typecheck), we end up aborting compilation early with only this un-informative message.

I'm not exactly sure _why_ this bug arises due to rust-lang#96591 -- perhaps valtrees are evaluated more eagerly than in the old system?

r? `@oli-obk` or `@lcnr` since y'all are familiar with const eval and reviewed rust-lang#96591, though feel free to reassign.

This is a regression from stable to beta, so I would be open to considering this for beta backport. It seems correct to me, especially given the improvements in the other UI tests this PR touches, but may have some side-effects that I'm unaware of...?
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jul 23, 2022
…-item, r=lcnr

Do not resolve associated const when there is no provided value

Fixes rust-lang#98629, since now we just delay a bug when we're not able to evaluate a const item due to the value not actually being provided by anything. This means compilation proceeds forward to where the "missing item in impl" error is emitted.

----

The root issue here is that when we're looking for the defining `LeafDef` in `resolve_associated_item`, we end up getting the trait's AssocItem instead of the impl's AssocItem (which does not exist). This resolution "succeeds" even if the trait's item has no default value, and then since this item has no value to evaluate, it turns into a const eval error.

This root issue becomes problematic (as in rust-lang#98629) when this const eval error happens in wfcheck (for example, due to normalizing the param-env of something that references this const). Since this happens sooner than the check that an impl actually provides all of the items that a trait requires (which happens during later typecheck), we end up aborting compilation early with only this un-informative message.

I'm not exactly sure _why_ this bug arises due to rust-lang#96591 -- perhaps valtrees are evaluated more eagerly than in the old system?

r? `@oli-obk` or `@lcnr` since y'all are familiar with const eval and reviewed rust-lang#96591, though feel free to reassign.

This is a regression from stable to beta, so I would be open to considering this for beta backport. It seems correct to me, especially given the improvements in the other UI tests this PR touches, but may have some side-effects that I'm unaware of...?
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jul 23, 2022
…-item, r=lcnr

Do not resolve associated const when there is no provided value

Fixes rust-lang#98629, since now we just delay a bug when we're not able to evaluate a const item due to the value not actually being provided by anything. This means compilation proceeds forward to where the "missing item in impl" error is emitted.

----

The root issue here is that when we're looking for the defining `LeafDef` in `resolve_associated_item`, we end up getting the trait's AssocItem instead of the impl's AssocItem (which does not exist). This resolution "succeeds" even if the trait's item has no default value, and then since this item has no value to evaluate, it turns into a const eval error.

This root issue becomes problematic (as in rust-lang#98629) when this const eval error happens in wfcheck (for example, due to normalizing the param-env of something that references this const). Since this happens sooner than the check that an impl actually provides all of the items that a trait requires (which happens during later typecheck), we end up aborting compilation early with only this un-informative message.

I'm not exactly sure _why_ this bug arises due to rust-lang#96591 -- perhaps valtrees are evaluated more eagerly than in the old system?

r? ``@oli-obk`` or ``@lcnr`` since y'all are familiar with const eval and reviewed rust-lang#96591, though feel free to reassign.

This is a regression from stable to beta, so I would be open to considering this for beta backport. It seems correct to me, especially given the improvements in the other UI tests this PR touches, but may have some side-effects that I'm unaware of...?
@ecnelises
Copy link
Contributor

For commit e14b34c , the two to_le() actually produces different result on big-endian from little-endian.

// Note: Don't use `StableHashResult` impl of `u64` here directly, since that
// would lead to endianness problems.
let hash: u128 = hasher.finish();
let hash_short = (hash.to_le() as u64).to_le();

Assume hash: u128=0x5e0f03940fda80bb6348c650c7b26618, then on LE, hash_short: u64 = 0x6348c650c7b26618, while BE hash_short: u64 = 0x5e0f03940fda80bb, which fails some unit tests on big endian targets because of hash mismatch.

If it's expected behavior to make hash value the same on BE/LE, code here should remove two to_le calls. We don't need to adjust endianness unless converting between multi-byte integer and byte sequences.

@b-naber
Copy link
Contributor Author

b-naber commented Oct 18, 2022

If it's expected behavior to make hash value the same on BE/LE, code here should remove two to_le calls. We don't need to adjust endianness unless converting between multi-byte integer and byte sequences.

I don't remember exactly what was going on (and I'd hate to read into this again), but we did need those calls in order to pass the debug tests on i686-msvc-1. Feel free to open a PR that removes those calls, though unless something else changed I don't think those tests will pass on that machine.

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 22, 2022
Remove byte swap of valtree hash on big endian

This addresses problem reported in rust-lang#103183. The code was originally introduced in rust-lang@e14b34c. (see rust-lang#96591)

On big-endian environment, this operation sequence actually put the other half from 128-bit result, thus we got different hash result on LE and BE.
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Oct 25, 2022
Remove byte swap of valtree hash on big endian

This addresses problem reported in #103183. The code was originally introduced in rust-lang/rust@e14b34c. (see rust-lang/rust#96591)

On big-endian environment, this operation sequence actually put the other half from 128-bit result, thus we got different hash result on LE and BE.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.