Skip to content

Conversation

@errge
Copy link
Contributor

@errge errge commented May 17, 2024

Trying to reproduce this RVO shows that actually the optimization that is used here is simple inlining. Once inlining is disabled, the addresses change, even with -O -C opt-level=3.

The return of the values is also never changed to an "efficient memcpy", but instead was returned on eax+edx, although this result is of course specific to the calling convention of the platform ABI, but no memcpy on the most popular amd64 architecture.

I don't think it's educationally important to teach RVO here, so I didn't go into any length trying to force Rust to do a real RVO (passing Point address into the function on the assembly level, so the function can fill in the addition). In my opinion the only important thing is, that if a student is actually clicking the Playground link and looks into the assembly, then our description should match the generated code. That's why I just fixed the content instead of trying to fix the example to be RVO.

Trying to reproduce this RVO shows that actually the optimization that
is used here is simple inlining.  Once inlining is disabled, the
addresses change, even with `-O -C opt-level=3`.

The return of the values is also never changed to an "efficient
memcpy", but instead was returned on eax+edx, although this result is
of course specific to the calling convention of the platform ABI, but
no memcpy on the most popular amd64 architecture.
Copy link
Collaborator

@djmitche djmitche left a comment

Choose a reason for hiding this comment

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

Looks good! I think the course should avoid getting too into the weeds on code generation in general. Folks who are interested in such things can dig down on their own.

@djmitche djmitche merged commit 889a4cb into google:main May 17, 2024
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

Successfully merging this pull request may close these issues.

2 participants