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

[BUG] Unexpected inability of compiler to optimize away moves #2410

Open
Brian-M-J opened this issue Apr 25, 2024 · 3 comments
Open

[BUG] Unexpected inability of compiler to optimize away moves #2410

Brian-M-J opened this issue Apr 25, 2024 · 3 comments
Labels
bug Something isn't working mojo Issues that are related to mojo mojo-repo Tag all issues with this label

Comments

@Brian-M-J
Copy link

Bug description

I was testing return value optimization in Mojo when I ran into some unexpected behaviour. For the first snippet linked below, the compiler is able to optimize away the move after returning from add_10 as demonstrated by the output:

Called constructor for DemoStruct(42)
Entering add_10 function
Called constructor for DemoStruct(52)
After calling add_10 function
DemoStruct(42)
Calling destructor for DemoStruct(42)
DemoStruct(52)
Calling destructor for DemoStruct(52)

But for the second snippet linked below, the compiler is unable to optimize away the move even though the behaviour of the function is still the same:

Called constructor for DemoStruct(42)
Entering add_10 function
Called constructor for DemoStruct(52)
Exiting add_10 function
Called move constructor for DemoStruct(52)
After calling add_10 function
DemoStruct(42)
Calling destructor for DemoStruct(42)
DemoStruct(52)
Calling destructor for DemoStruct(52)

It's better that programmers shouldn't worry about reordering statements introducing hidden costs and affecting performance.

Steps to reproduce

Move optimized away: https://gist.github.com/modularbot/117c6679aea61ee5143ada73b3f2eb5d

Move not optimized away: https://gist.github.com/modularbot/c60ce97f28c5dcf04233aa601814c8e3

System information

The Mojo playground as of 25-04-2024
@Brian-M-J Brian-M-J added bug Something isn't working mojo Issues that are related to mojo labels Apr 25, 2024
@Brian-M-J
Copy link
Author

Brian-M-J commented Apr 26, 2024

Another place the compiler should be able to optimize away moves is for functions/methods that take their inputs as inout/borrowed vs owned (e.g. for in-place functional updates):

When the argument is taken as inout and mutated, no moves occur:

Called constructor for DemoStruct(42)
Entering modify function
Exiting modify function
Entering modify function
Exiting modify function
DemoStruct(20)
Calling destructor for DemoStruct(20)

When the input is taken as owned and mutated, moves do occur even though the effect of the code is the same:

Called constructor for DemoStruct(42)
Entering modify function
Exiting modify function
Called move constructor for DemoStruct(10)
Entering modify function
Exiting modify function
Called move constructor for DemoStruct(20)
DemoStruct(20)
Calling destructor for DemoStruct(20)

This introduces hidden run time costs for programmers who prefer pure functions/methods. Also, if they want to use pure functions when working with non-movable types (in the very few cases where it can be done), it is currently not possible.

Even if there is no mutation, borrowing does not call __moveinit__:

Called constructor for DemoStruct(42)
Entering display function
DemoStruct(42)
Exiting display function
Entering display function
DemoStruct(42)
Exiting display function
Calling destructor for DemoStruct(42)

...but passing through owned does:

Called constructor for DemoStruct(42)
Entering display function
DemoStruct(42)
Exiting display function
Called move constructor for DemoStruct(42)
Entering display function
DemoStruct(42)
Exiting display function
Called move constructor for DemoStruct(42)
Calling destructor for DemoStruct(42)

Also see p2666 - Last use optimization:

Last use of the value stored in one of these ways refers to not only to locations where the
compiler can deduce that the variable is not at all used later, but also that the next use is definitely
an assignment of a new value. This covers the important case of modifying a value like in
name = toUpper(name); where the old contents of name is allowed to be moved into toUpper as
the next use of name is it being assigned the result of toUpper.

@nmsmith
Copy link
Contributor

nmsmith commented Apr 26, 2024

Just FYI, Mojo is likely going to offer explicit return value optimization, which will be useful in cases where you really, really want to make sure that a return value isn't moved.

Not that this means general improvements to code generation aren't valuable. Those are very nice as well. 🙂

@Brian-M-J
Copy link
Author

Just FYI, Mojo is likely going to offer explicit return value optimization, which will be useful in cases where you really, really want to make sure that a return value isn't moved.

Not that this means general improvements to code generation aren't valuable. Those are very nice as well. 🙂

No, this is great. I much prefer the syntactically explicit, guaranteed optimizations that Mojo offers. It removes the worries of relying on the compiler being "sufficiently smart" such that it can recognize the opportunity to perform these optimizations (parallelizing, vectorizing, RVO) in all cases. No more relying on "compiler magic".

General codegen improvements like these just ensure that one doesn't have to perform a lot of syntactic ceremony to get good enough performance.

@ematejska ematejska added the mojo-repo Tag all issues with this label label Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working mojo Issues that are related to mojo mojo-repo Tag all issues with this label
Projects
None yet
Development

No branches or pull requests

3 participants