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

Place semantics of function parameters (optimizing pass-by-value) #429

Closed
CAD97 opened this issue Jul 8, 2023 · 6 comments
Closed

Place semantics of function parameters (optimizing pass-by-value) #429

CAD97 opened this issue Jul 8, 2023 · 6 comments

Comments

@CAD97
Copy link

CAD97 commented Jul 8, 2023

This ties into place/address uniqueness discussions.

Consider that we have some type Data which has pass-by-reference ABI and isn't Copy. We have code with the shape:

extern fn consume(data: Data);

fn entry(data: Box<Data>) {
    consume(*data);
}

Using &move to represent the ABI, this lowers to roughly look like:

fn entry(data: Box<Data>) {
    'bb0: {
        let tmp = *data;
        consume(&move tmp) -> [return: 'bb1, unwind: 'bb2];
    }
    'bb1: {
        Drop::drop(&mut data); // outer drop
        return;
    }
    'bb2: {
        Drop::drop(&mut data); // outer drop
        k#resume_unwind;
    }
}
Optimized MIR
fn entry(_1: Box<Data>) -> () {
    debug x => _1;
    let mut _0: ();
    let _2: ();
    let mut _3: Data;
    let mut _4: &mut std::boxed::Box<Data>;
    let mut _5: ();
    let mut _6: &mut std::boxed::Box<Data>;
    let mut _7: ();
    let mut _8: *const Data;

    bb0: {
        StorageLive(_3);
        _8 = (((_1.0: std::ptr::Unique<Data>).0: std::ptr::NonNull<Data>).0: *const Data);
        _3 = move (*_8);
        _2 = consume(move _3) -> [return: bb1, unwind: bb4];
    }

    bb1: {
        StorageDead(_3);
        _4 = &mut _1;
        _5 = <Box<Data> as Drop>::drop(move _4) -> bb3;
    }

    bb2 (cleanup): {
        resume;
    }

    bb3: {
        return;
    }

    bb4 (cleanup): {
        _6 = &mut _1;
        _7 = <Box<Data> as Drop>::drop(move _6) -> [return: bb2, unwind terminate];
    }
}

My question: would it be a valid optimization to remove the temporary place and instead directly call consume(&move *data)? [context]

Reasons for:

  • The boxed place, by virtue of being passed as a Box, is known to be uniquely accessible by fn entry.
  • As such, no other code has a valid pointer which would alias the potentially reused place.
  • Eliding the copy would be a desirable optimization for address insensitive data. Which, given the lack of Pin involved, SHOULD be the case.

Reasons against:

  • Opsem doesn't care about SHOULD.
  • The boxed place is semantically a distinct place from the temporary place.
  • The boxed place (despite being unused and inaccessible) is still live across the region where the temporary place is live.
  • Code could observe the address colocation of the two live places if it knows the address of the boxed place.

Obviously, being able to prove more about the code (i.e. that the address of one or the other place isn't observed) would permit this copy elision optimization. I'm specifically curious about the know-nothing case.

If we categorically forbid live places from (observably) having the same address, then I believe this optimization would indeed be incorrect. If we can't perform this optimization automatically, that could serve as partial motivation for the introduction of a surface level &move which would permit such (well-scoped) place colocation.

(This is not the place to discuss the exact semantics of &move. Just to determine if something like &move would be required to do this optimization of logical ownership transfer but concrete place reuse.)

@bjorn3
Copy link
Member

bjorn3 commented Jul 8, 2023

For unsized values this is already effectively guaranteed to not copy. Copying unsized values when doing argument passing is not possible without unsized locals, which no backend other than the LLVM backend supports.

@bjorn3
Copy link
Member

bjorn3 commented Jul 8, 2023

Also there has been a lot of discussion about the semantics of calls before.

@CAD97
Copy link
Author

CAD97 commented Jul 9, 2023

If by value unsized parameters get (necessarily) passed in their existing place rather than a fresh one, that definitely pushes me towards thinking that temporary/call semantics should be such that sized values also have the option of place colocation.

Although:

  • Sized values can have alternate ABIs (e.g. scalar/scalar-pair get passed in registers), so it needs to be nondeterministic; and
  • There's no strict need for sized and unsized values to behave identically, since these decisions are done on instantiated/monomorphized code rather than generic.

I'd expect this to be formalized along the lines of that a parameter of "kind" place is handled directly as a place (when the generic type is not Copy) rather than coerced to a (temporary) value, and that using a place argument permits permits the arguments' place to be colocated with the passed place. Allowing this probably requires putting protectors on the passed place.

In MIR terms

Currently, the MIR for consume(*data) looks like

_2: *const Data = ((((
    _1: std::boxed::Box<Data>)
    .0: std::ptr::Unique<Data>)
    .0: std::ptr::NonNull<Data>)
    .0: *const Data);
_3: Data = move (*_2);
_4: () = consume(move _3) -> [return: bb1, unwind: bb2];

Instead of introducing the temporary place _3, we would instead lower as

_2: *const Data = ((((
    _1: std::boxed::Box<Data>)
    .0: std::ptr::Unique<Data>)
    .0: std::ptr::NonNull<Data>)
    .0: *const Data);
_4: () = consume(move (*_2)) -> [return: bb1, unwind: bb2];

This is, if I'm not mistaken, the MIR already produced for unsized parameters. Though somewhat interestingly, there seems to be a difference in how the box itself gets treated? (In the sized case, it's left in the argument place, but in the unsized case, it's moved into a fresh place. Potentially related to handling partial uninit and reinit?)

If I hack this in with #[custom_mir], it already has the effect of reusing the place (passing the pointer directly) for by-ref ABI or making a copy for by-scalar ABI.


This has a somewhat unfortunate effect that extracting a temporary variable has a pessimizing effect, but when dealing with address stability/disjointedness, it's to be expected. (&move would permit extracting a name without forcing a copy.)

I've been meaning to familiarize myself with big-picture how MIR is generated; perhaps I could experiment with making this change in MIR generation (but still gated behind unsized_fn_params, probably).

Since it's on my mind, this would be another difference with how become (tail) calls would be treated.

@CAD97 CAD97 changed the title Optimizing pass-by-value Place semantics of function parameters (optimizing pass-by-value) Jul 9, 2023
@CAD97
Copy link
Author

CAD97 commented Jul 10, 2023

Related to (and potentially duplicate of) #416 and/or the other issues it trees out to.

@RalfJung
Copy link
Member

Yeah I think this is basically a duplicate of #416 and rust-lang/rust#71117. Should we close it as a duplicate?

FWIW, I just opened rust-lang/rust#113569 which I think justifies the semantics you describe except for address identity issues. The MIR way of expressing this is awkward (I'm hoping to sketch a syntax better reflecting the semantics in MiniRust); basically if the argument is of the shape Move(place) then we don't treat it like an operand -- really the list of function arguments should be "list of either operands (to copy) or places (to optionally use for in-place argument passing)"; an operand is a value expression and shouldn't have a "location in memory" or anything like that.

@CAD97
Copy link
Author

CAD97 commented Jul 11, 2023

Yeah, I think between those what I was looking at here gets covered. The core question of "can we allow this" is answered and #416 serves fine to track the "do we" part.

@CAD97 CAD97 closed this as completed Jul 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants