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

Add ptr::read_move #113066

Closed
wants to merge 4 commits into from
Closed

Add ptr::read_move #113066

wants to merge 4 commits into from

Conversation

newpavlov
Copy link
Contributor

Experimental support for reading from pointer by moving from it. It's necessary to allow compiler to reuse memory behind the "read" pointer. See this discussion for more context: https://internals.rust-lang.org/t/19038

This implementation mirrors ptr::read, but with the small change in the intrinsic implementation.

@rustbot
Copy link
Collaborator

rustbot commented Jun 26, 2023

r? @cuviper

(rustbot has picked a reviewer for you, use r? to override)

@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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 26, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jun 26, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@newpavlov
Copy link
Contributor Author

r? @scottmcm

@rustbot rustbot assigned scottmcm and unassigned cuviper Jun 26, 2023
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@scottmcm
Copy link
Member

scottmcm commented Jun 26, 2023

Hmm, so I have no problem with this experimentally, but while I mentioned it in IRLO, I don't actually know it'll actually be useful right now. Could you try it out with custom MIR on your end and report whether it actually helps you?

You can write this on nightly:

#![feature(core_intrinsics)]
#![feature(custom_mir)]
use std::intrinsics::mir::*;

#[custom_mir(dialect = "runtime", phase = "optimized")]
pub fn experimental_read_via_move<T>(p: *const T) -> T {
    mir!({
        RET = Move(*p);
        Return()
    })   
}

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=fe812dfa50fe8f0bec5cb26407706537

I figure there's probably some complicated opsem questions about what move even does before this could be stabilized. (copy *p clearly doesn't change the place, but for move *p to be useful, it probably has to do something, but I don't know that we know exactly what that would be.)

@scottmcm scottmcm 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 Jun 26, 2023
///
/// * `src` must point to a properly initialized value of type `T`.
///
/// * `src` must not be read or written while the return value exists.
Copy link
Member

Choose a reason for hiding this comment

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

I am ignorant about MIR move semantics.

Does it apply to T: Copy too? How does that interact with freeing the allocation from which the value has been read?

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-14 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
   Compiling unwind v0.0.0 (/checkout/library/unwind)
error: function has missing stability attribute
    --> library/core/src/ptr/mod.rs:1204:1
     |
1204 | / pub const unsafe fn read_move<T>(src: *mut T) -> T {
1205 | |     // SAFETY: the caller must guarantee that `src` is valid for reads.
1207 | |         assert_unsafe_precondition!(
...    |
1212 | |     }
1213 | | }
1213 | | }
     | |_^

error[E0308]: intrinsic has wrong type
    --> library/core/src/intrinsics.rs:2283:5
     |
2283 |     pub fn read_via_move<T>(ptr: *mut T) -> T;
     |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ types differ in mutability
     |
     = note: expected fn pointer `unsafe extern "rust-intrinsic" fn(*mut T) -> T`
                found fn pointer `unsafe extern "rust-intrinsic" fn(*const P0) -> P0`
For more information about this error, try `rustc --explain E0308`.
error: could not compile `core` (lib) due to 2 previous errors
Build completed unsuccessfully in 0:03:37

@newpavlov
Copy link
Contributor Author

@scottmcm
Yeah, it looks like "moving" from pointer does not help: https://rust.godbolt.org/z/b5en6EMY5

Unfortunately, it looks like the feature will need much deeper compiler changes.

@newpavlov newpavlov closed this Jun 26, 2023
@newpavlov newpavlov deleted the read_move branch June 26, 2023 17:59
@scottmcm
Copy link
Member

I think there's an opsem question here first, even. Because it's unclear to me whether the place is even allowed to be reused, because I don't know if you're allowed to keep that pointer to the place and read it later, despite having moved out of it already. (And, more importantly, if that's not allowed, what it's doing that doesn't break moving out of the inside of a nice-optimized Option.)

@newpavlov
Copy link
Contributor Author

Moving or reading a pointer may not be a good abstraction in the end. Something like *own T (mentioned in the IRLO discussion) which would be allowed to be used in place of T could be a better approach. Depending on ABI compiler will either read the referenced value or will use the pointer directly. Though I am not familiar enough with MIR and LLVM to begin drafting the necessary semantics and potential implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants