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

ABI: Ownership transfer through pointer involves unnecessary copies #86711

Open
CryZe opened this issue Jun 29, 2021 · 1 comment
Open

ABI: Ownership transfer through pointer involves unnecessary copies #86711

CryZe opened this issue Jun 29, 2021 · 1 comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. I-slow Issue: Problems and improvements with respect to performance of generated code.

Comments

@CryZe
Copy link
Contributor

CryZe commented Jun 29, 2021

Let's say we have a reasonably large struct that doesn't get passed through registers:

pub struct Color {
    red: f64,
    green: f64,
    blue: f64,
    alpha: f64,
}

Then we have some method on it that requires ownership transfer:

impl Color {
    #[inline(never)]
    fn to_rgba8(self) -> [u8; 4] {
        [
            (255.0 * self.red) as u8,
            (255.0 * self.green) as u8,
            (255.0 * self.blue) as u8,
            (255.0 * self.alpha) as u8,
        ]
    }
}

And then we have some sort of function that just passes ownership through to that method:

pub fn convert(color: Color) -> [u8; 4] {
    color.to_rgba8()
}

In C you would expect a "copy" of the struct to get passed into the function, as we pass the struct by value. And that's exactly what Rust does here as well. It copies the entire struct onto the stack and then passes a pointer to the copy into the function:

example::convert:
        sub     rsp, 40
        movups  xmm0, xmmword ptr [rdi]
        movups  xmm1, xmmword ptr [rdi + 16]
        movaps  xmmword ptr [rsp + 16], xmm1
        movaps  xmmword ptr [rsp], xmm0
        mov     rdi, rsp
        call    example::Color::to_rgba8
        add     rsp, 40
        ret

However Rust has a unique notion of ownership, where we know that color can't be used anymore after calling the method. So no actual copy should ever be necessary for a non-Copy type. Though technically even on a Copy type no such additional copy should be necessary if it's the last use.

https://rust.godbolt.org/z/135z7aK3K

@jonas-schievink jonas-schievink added C-enhancement Category: An issue proposing an enhancement or a PR with one. I-slow Issue: Problems and improvements with respect to performance of generated code. labels Jun 29, 2021
@jonas-schievink
Copy link
Contributor

MIR generated for convert:

fn convert(_1: Color) -> [u8; 4] {
    debug color => _1;                   // in scope 0 at /app/example.rs:20:16: 20:21
    let mut _0: [u8; 4];                 // return place in scope 0 at /app/example.rs:20:33: 20:40
    let mut _2: Color;                   // in scope 0 at /app/example.rs:21:5: 21:10

    bb0: {
        StorageLive(_2);                 // scope 0 at /app/example.rs:21:5: 21:10
        _2 = move _1;                    // scope 0 at /app/example.rs:21:5: 21:10
        _0 = Color::to_rgba8(move _2) -> bb1; // scope 0 at /app/example.rs:21:5: 21:21
    }

    bb1: {
        StorageDead(_2);                 // scope 0 at /app/example.rs:21:20: 21:21
        return;                          // scope 0 at /app/example.rs:22:2: 22:2
    }
}

Seems like this would be easy to handle with intra-block copy propagation, or the existing dest_prop pass (though it doesn't seem to handle this case just yet).

facebook-github-bot pushed a commit to facebook/starlark-rust that referenced this issue Jan 17, 2022
Summary:
Pass call arguments as `&Arguments` instead of `Arguments`.

The problem can be illustrated by [this example](https://rust.godbolt.org/z/x74YzsWKf).

I hope one day such low level hacks will be not be necessary.

[Rust issue](rust-lang/rust#86711).

Reviewed By: ndmitchell

Differential Revision: D33589608

fbshipit-source-id: 9887c8d690e29302f23392c7ac247b57a487a327
facebook-github-bot pushed a commit to facebookincubator/antlir that referenced this issue Jan 17, 2022
Summary:
Pass call arguments as `&Arguments` instead of `Arguments`.

The problem can be illustrated by [this example](https://rust.godbolt.org/z/x74YzsWKf).

I hope one day such low level hacks will be not be necessary.

[Rust issue](rust-lang/rust#86711).

Reviewed By: ndmitchell

Differential Revision: D33589608

fbshipit-source-id: 9887c8d690e29302f23392c7ac247b57a487a327
vmagro pushed a commit to vmagro/antlir that referenced this issue Jan 20, 2022
Summary:
Pass call arguments as `&Arguments` instead of `Arguments`.

The problem can be illustrated by [this example](https://rust.godbolt.org/z/x74YzsWKf).

I hope one day such low level hacks will be not be necessary.

[Rust issue](rust-lang/rust#86711).

Reviewed By: ndmitchell

Differential Revision: D33589608

fbshipit-source-id: 9887c8d690e29302f23392c7ac247b57a487a327
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. I-slow Issue: Problems and improvements with respect to performance of generated code.
Projects
None yet
Development

No branches or pull requests

2 participants