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

explain why Rvalue::Len still exists #129939

Merged
merged 1 commit into from
Sep 6, 2024
Merged

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Sep 3, 2024

I just spent a bit of time trying to remove this until I realized why that's non-trivial. Let's document that for the next person. :)

@rustbot
Copy link
Collaborator

rustbot commented Sep 3, 2024

r? @cjgillot

rustbot has assigned @cjgillot.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot
Copy link
Collaborator

rustbot commented Sep 3, 2024

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 3, 2024
@compiler-errors
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 3, 2024

📌 Commit 98f74b4 has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 3, 2024
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Sep 3, 2024
…rors

explain why Rvalue::Len still exists

I just spent a bit of time trying to remove this until I realized why that's non-trivial. Let's document that for the next person. :)
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 3, 2024
…llaumeGomez

Rollup of 7 pull requests

Successful merges:

 - rust-lang#127021 (Add target support for RTEMS Arm)
 - rust-lang#128871 (bypass linker configuration and cross target check for specific commands)
 - rust-lang#129471 ([rustdoc] Sort impl associated items by kinds and then by appearance)
 - rust-lang#129529 (Add test to build crates used by r-a on stable)
 - rust-lang#129706 (Rename dump of coroutine by-move-body to be more consistent, fix ICE in dump_mir)
 - rust-lang#129796 (Unify scraped examples with other code examples)
 - rust-lang#129939 (explain why Rvalue::Len still exists)

r? `@ghost`
`@rustbot` modify labels: rollup
tgross35 added a commit to tgross35/rust that referenced this pull request Sep 4, 2024
…rors

explain why Rvalue::Len still exists

I just spent a bit of time trying to remove this until I realized why that's non-trivial. Let's document that for the next person. :)
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 4, 2024
Rollup of 8 pull requests

Successful merges:

 - rust-lang#101339 (enable -Zrandomize-layout in debug CI builds )
 - rust-lang#127021 (Add target support for RTEMS Arm)
 - rust-lang#128871 (bypass linker configuration and cross target check for specific commands)
 - rust-lang#129471 ([rustdoc] Sort impl associated items by kinds and then by appearance)
 - rust-lang#129529 (Add test to build crates used by r-a on stable)
 - rust-lang#129624 (Adjust `memchr` pinning and run `cargo update`)
 - rust-lang#129796 (Unify scraped examples with other code examples)
 - rust-lang#129939 (explain why Rvalue::Len still exists)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 4, 2024
Rollup of 8 pull requests

Successful merges:

 - rust-lang#101339 (enable -Zrandomize-layout in debug CI builds )
 - rust-lang#127021 (Add target support for RTEMS Arm)
 - rust-lang#128871 (bypass linker configuration and cross target check for specific commands)
 - rust-lang#129471 ([rustdoc] Sort impl associated items by kinds and then by appearance)
 - rust-lang#129529 (Add test to build crates used by r-a on stable)
 - rust-lang#129624 (Adjust `memchr` pinning and run `cargo update`)
 - rust-lang#129796 (Unify scraped examples with other code examples)
 - rust-lang#129939 (explain why Rvalue::Len still exists)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 4, 2024
Rollup of 8 pull requests

Successful merges:

 - rust-lang#101339 (enable -Zrandomize-layout in debug CI builds )
 - rust-lang#127021 (Add target support for RTEMS Arm)
 - rust-lang#128871 (bypass linker configuration and cross target check for specific commands)
 - rust-lang#129471 ([rustdoc] Sort impl associated items by kinds and then by appearance)
 - rust-lang#129529 (Add test to build crates used by r-a on stable)
 - rust-lang#129624 (Adjust `memchr` pinning and run `cargo update`)
 - rust-lang#129796 (Unify scraped examples with other code examples)
 - rust-lang#129939 (explain why Rvalue::Len still exists)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 5, 2024
Rollup of 8 pull requests

Successful merges:

 - rust-lang#101339 (enable -Zrandomize-layout in debug CI builds )
 - rust-lang#127021 (Add target support for RTEMS Arm)
 - rust-lang#128871 (bypass linker configuration and cross target check for specific commands)
 - rust-lang#129471 ([rustdoc] Sort impl associated items by kinds and then by appearance)
 - rust-lang#129529 (Add test to build crates used by r-a on stable)
 - rust-lang#129624 (Adjust `memchr` pinning and run `cargo update`)
 - rust-lang#129796 (Unify scraped examples with other code examples)
 - rust-lang#129939 (explain why Rvalue::Len still exists)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 5, 2024
…rors

explain why Rvalue::Len still exists

I just spent a bit of time trying to remove this until I realized why that's non-trivial. Let's document that for the next person. :)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 5, 2024
…rors

explain why Rvalue::Len still exists

I just spent a bit of time trying to remove this until I realized why that's non-trivial. Let's document that for the next person. :)
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 5, 2024
…iaskrgr

Rollup of 12 pull requests

Successful merges:

 - rust-lang#128919 (Add an internal lint that warns when accessing untracked data)
 - rust-lang#129471 ([rustdoc] Sort impl associated items by kinds and then by appearance)
 - rust-lang#129653 (clarify that addr_of creates read-only pointers)
 - rust-lang#129775 (bootstrap: Try to track down why `initial_libdir` sometimes fails)
 - rust-lang#129939 (explain why Rvalue::Len still exists)
 - rust-lang#129940 (s390x: Fix a regression related to backchain feature)
 - rust-lang#129942 (copy rustc rustlib artifacts from ci-rustc)
 - rust-lang#129943 (use the bootstrapped compiler for `test-float-parse` test)
 - rust-lang#129944 (Add compat note for trait solver change)
 - rust-lang#129947 (Add digit separators in `Duration` examples)
 - rust-lang#129955 (Temporarily remove fmease from the review rotation)
 - rust-lang#129957 (forward linker option to lint-docs)

r? `@ghost`
`@rustbot` modify labels: rollup
@scottmcm
Copy link
Member

scottmcm commented Sep 5, 2024

I have a branch somewhere that removes it -- it's not that hard to change the emitting code to just do PtrMetadata -- but the bigger problem I hit was things like the dataflow framework. #126541 made some progress there by making GVN handle it, but as you found out, it doesn't seem trivial to nuke it.

(MIR building has enough types that when it needs a length for indexing it could emit the size directly for arrays or the right PtrMetadata for slices without much trouble.)

@RalfJung
Copy link
Member Author

RalfJung commented Sep 5, 2024

(MIR building has enough types that when it needs a length for indexing it could emit the size directly for arrays or the right PtrMetadata for slices without much trouble.)

That's what I tired, but it would have to add a temporary to hold a pointer to the place, to pass it to PtrMetadata -- and that seemed like a non-trivial-enough change that I stopped there.

@@ -1307,6 +1307,9 @@ pub enum Rvalue<'tcx> {
/// If the type of the place is an array, this is the array length. For slices (`[T]`, not
/// `&[T]`) this accesses the place's metadata to determine the length. This rvalue is
/// ill-formed for places of other types.
///
/// This cannot be a `UnOp(PtrMetadata, _)` because that expects a value, and we only
/// have a place, and `UnOp(PtrMetadata, RawPtr(place))` is not a thing.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other complication here, dunno whether it's worth mentioning, is that PtrMetadata needs a wide pointer, so if the place is an array you also need to unsize it to slice.

BTW, are there [ST]B issues with adding a short-lived raw-ref to use PtrMetadata if the place is something like (*_2).1? Or is it just that it's a bit annoying to make the temporary?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other complication here, dunno whether it's worth mentioning, is that PtrMetadata needs a wide pointer, so if the place is an array you also need to unsize it to slice.

Not really, as you said above -- if the place is an array we can just emit a constant instead.

BTW, are there [ST]B issues with adding a short-lived raw-ref to use PtrMetadata if the place is something like (*_2).1? Or is it just that it's a bit annoying to make the temporary?

There also shouldn't be any -- creating a raw pointer you never use should be effectively a NOP.

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 5, 2024
…iaskrgr

Rollup of 12 pull requests

Successful merges:

 - rust-lang#128919 (Add an internal lint that warns when accessing untracked data)
 - rust-lang#129471 ([rustdoc] Sort impl associated items by kinds and then by appearance)
 - rust-lang#129653 (clarify that addr_of creates read-only pointers)
 - rust-lang#129775 (bootstrap: Try to track down why `initial_libdir` sometimes fails)
 - rust-lang#129939 (explain why Rvalue::Len still exists)
 - rust-lang#129940 (s390x: Fix a regression related to backchain feature)
 - rust-lang#129942 (copy rustc rustlib artifacts from ci-rustc)
 - rust-lang#129943 (use the bootstrapped compiler for `test-float-parse` test)
 - rust-lang#129944 (Add compat note for trait solver change)
 - rust-lang#129947 (Add digit separators in `Duration` examples)
 - rust-lang#129955 (Temporarily remove fmease from the review rotation)
 - rust-lang#129957 (forward linker option to lint-docs)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 5, 2024
…rors

explain why Rvalue::Len still exists

I just spent a bit of time trying to remove this until I realized why that's non-trivial. Let's document that for the next person. :)
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 5, 2024
…iaskrgr

Rollup of 12 pull requests

Successful merges:

 - rust-lang#128919 (Add an internal lint that warns when accessing untracked data)
 - rust-lang#129472 (fix ICE when `asm_const` and `const_refs_to_static` are combined)
 - rust-lang#129653 (clarify that addr_of creates read-only pointers)
 - rust-lang#129775 (bootstrap: Try to track down why `initial_libdir` sometimes fails)
 - rust-lang#129939 (explain why Rvalue::Len still exists)
 - rust-lang#129940 (s390x: Fix a regression related to backchain feature)
 - rust-lang#129942 (copy rustc rustlib artifacts from ci-rustc)
 - rust-lang#129943 (use the bootstrapped compiler for `test-float-parse` test)
 - rust-lang#129944 (Add compat note for trait solver change)
 - rust-lang#129947 (Add digit separators in `Duration` examples)
 - rust-lang#129955 (Temporarily remove fmease from the review rotation)
 - rust-lang#129957 (forward linker option to lint-docs)

Failed merges:

 - rust-lang#129471 ([rustdoc] Sort impl associated items by kinds and then by appearance)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 5, 2024
…iaskrgr

Rollup of 12 pull requests

Successful merges:

 - rust-lang#128919 (Add an internal lint that warns when accessing untracked data)
 - rust-lang#129472 (fix ICE when `asm_const` and `const_refs_to_static` are combined)
 - rust-lang#129653 (clarify that addr_of creates read-only pointers)
 - rust-lang#129775 (bootstrap: Try to track down why `initial_libdir` sometimes fails)
 - rust-lang#129939 (explain why Rvalue::Len still exists)
 - rust-lang#129940 (s390x: Fix a regression related to backchain feature)
 - rust-lang#129942 (copy rustc rustlib artifacts from ci-rustc)
 - rust-lang#129943 (use the bootstrapped compiler for `test-float-parse` test)
 - rust-lang#129944 (Add compat note for trait solver change)
 - rust-lang#129947 (Add digit separators in `Duration` examples)
 - rust-lang#129955 (Temporarily remove fmease from the review rotation)
 - rust-lang#129957 (forward linker option to lint-docs)

Failed merges:

 - rust-lang#129471 ([rustdoc] Sort impl associated items by kinds and then by appearance)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 5, 2024
…iaskrgr

Rollup of 12 pull requests

Successful merges:

 - rust-lang#128919 (Add an internal lint that warns when accessing untracked data)
 - rust-lang#129472 (fix ICE when `asm_const` and `const_refs_to_static` are combined)
 - rust-lang#129653 (clarify that addr_of creates read-only pointers)
 - rust-lang#129775 (bootstrap: Try to track down why `initial_libdir` sometimes fails)
 - rust-lang#129939 (explain why Rvalue::Len still exists)
 - rust-lang#129940 (s390x: Fix a regression related to backchain feature)
 - rust-lang#129942 (copy rustc rustlib artifacts from ci-rustc)
 - rust-lang#129943 (use the bootstrapped compiler for `test-float-parse` test)
 - rust-lang#129944 (Add compat note for trait solver change)
 - rust-lang#129947 (Add digit separators in `Duration` examples)
 - rust-lang#129955 (Temporarily remove fmease from the review rotation)
 - rust-lang#129957 (forward linker option to lint-docs)

Failed merges:

 - rust-lang#129471 ([rustdoc] Sort impl associated items by kinds and then by appearance)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 5, 2024
…iaskrgr

Rollup of 11 pull requests

Successful merges:

 - rust-lang#128919 (Add an internal lint that warns when accessing untracked data)
 - rust-lang#129472 (fix ICE when `asm_const` and `const_refs_to_static` are combined)
 - rust-lang#129653 (clarify that addr_of creates read-only pointers)
 - rust-lang#129775 (bootstrap: Try to track down why `initial_libdir` sometimes fails)
 - rust-lang#129939 (explain why Rvalue::Len still exists)
 - rust-lang#129942 (copy rustc rustlib artifacts from ci-rustc)
 - rust-lang#129943 (use the bootstrapped compiler for `test-float-parse` test)
 - rust-lang#129944 (Add compat note for trait solver change)
 - rust-lang#129947 (Add digit separators in `Duration` examples)
 - rust-lang#129955 (Temporarily remove fmease from the review rotation)
 - rust-lang#129957 (forward linker option to lint-docs)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 5, 2024
…iaskrgr

Rollup of 11 pull requests

Successful merges:

 - rust-lang#128919 (Add an internal lint that warns when accessing untracked data)
 - rust-lang#129472 (fix ICE when `asm_const` and `const_refs_to_static` are combined)
 - rust-lang#129653 (clarify that addr_of creates read-only pointers)
 - rust-lang#129775 (bootstrap: Try to track down why `initial_libdir` sometimes fails)
 - rust-lang#129939 (explain why Rvalue::Len still exists)
 - rust-lang#129942 (copy rustc rustlib artifacts from ci-rustc)
 - rust-lang#129943 (use the bootstrapped compiler for `test-float-parse` test)
 - rust-lang#129944 (Add compat note for trait solver change)
 - rust-lang#129947 (Add digit separators in `Duration` examples)
 - rust-lang#129955 (Temporarily remove fmease from the review rotation)
 - rust-lang#129957 (forward linker option to lint-docs)

r? `@ghost`
`@rustbot` modify labels: rollup
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Sep 6, 2024
…rors

explain why Rvalue::Len still exists

I just spent a bit of time trying to remove this until I realized why that's non-trivial. Let's document that for the next person. :)
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Sep 6, 2024
…rors

explain why Rvalue::Len still exists

I just spent a bit of time trying to remove this until I realized why that's non-trivial. Let's document that for the next person. :)
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 6, 2024
…iaskrgr

Rollup of 11 pull requests

Successful merges:

 - rust-lang#128919 (Add an internal lint that warns when accessing untracked data)
 - rust-lang#129472 (fix ICE when `asm_const` and `const_refs_to_static` are combined)
 - rust-lang#129653 (clarify that addr_of creates read-only pointers)
 - rust-lang#129775 (bootstrap: Try to track down why `initial_libdir` sometimes fails)
 - rust-lang#129939 (explain why Rvalue::Len still exists)
 - rust-lang#129942 (copy rustc rustlib artifacts from ci-rustc)
 - rust-lang#129943 (use the bootstrapped compiler for `test-float-parse` test)
 - rust-lang#129944 (Add compat note for trait solver change)
 - rust-lang#129947 (Add digit separators in `Duration` examples)
 - rust-lang#129955 (Temporarily remove fmease from the review rotation)
 - rust-lang#129957 (forward linker option to lint-docs)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 2efefe6 into rust-lang:master Sep 6, 2024
6 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 6, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 6, 2024
…kingjubilee

Rollup of 14 pull requests

Successful merges:

 - rust-lang#128919 (Add an internal lint that warns when accessing untracked data)
 - rust-lang#129021 (Check WF of source type's signature on fn pointer cast)
 - rust-lang#129472 (fix ICE when `asm_const` and `const_refs_to_static` are combined)
 - rust-lang#129653 (clarify that addr_of creates read-only pointers)
 - rust-lang#129775 (bootstrap: Try to track down why `initial_libdir` sometimes fails)
 - rust-lang#129781 (Make `./x.py <cmd> compiler/<crate>` aware of the crate's features)
 - rust-lang#129939 (explain why Rvalue::Len still exists)
 - rust-lang#129942 (copy rustc rustlib artifacts from ci-rustc)
 - rust-lang#129944 (Add compat note for trait solver change)
 - rust-lang#129947 (Add digit separators in `Duration` examples)
 - rust-lang#129955 (Temporarily remove fmease from the review rotation)
 - rust-lang#129957 (forward linker option to lint-docs)
 - rust-lang#129969 (Make `Ty::boxed_ty` return an `Option`)
 - rust-lang#129995 (Remove wasm32-wasip2's tier 2 status from release notes)

r? `@ghost`
`@rustbot` modify labels: rollup
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 6, 2024
Rollup merge of rust-lang#129939 - RalfJung:rvalue-len, r=compiler-errors

explain why Rvalue::Len still exists

I just spent a bit of time trying to remove this until I realized why that's non-trivial. Let's document that for the next person. :)
@RalfJung RalfJung deleted the rvalue-len branch September 9, 2024 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

6 participants