explain why Rvalue::Len still exists by RalfJung · Pull Request #129939 · rust-lang/rust (original) (raw)

📌 Commit 98f74b4 has been approved by compiler-errors

It is now in the queue for this repository.

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

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request

…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

…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

…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

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

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

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

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

…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

…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

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.)

(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.

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?

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

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

…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

…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

…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

…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

bors added a commit to rust-lang-ci/rust that referenced this pull request

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request

…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

…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

bors added a commit to rust-lang-ci/rust that referenced this pull request

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request