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

Help compilers optimize Object::cast_to() #82903

Merged
merged 1 commit into from
Mar 6, 2024

Conversation

RandomShaper
Copy link
Member

This PR adds an explicit null-check to Object::cast_to(). At -O2 both GCC and Clang seem to do the check themselves to avoid the call to the implementation of dynamic_cast when there's no object. However, MSVC (/O2) doesn't add a check and will do the call regardless.

All the three compilers can fuse the check here with any explicit check done at the call site. That said, if the contract of cast_to() is that it can take null just fine and any optimization is up to it (it's inlined anyway), the callers shouldn't be trying to outsmart it (or the compiler).

One could argue that in cases where there's an object for sure, the check is superfluous. However, in compilers that don't add it before the call to dynamic_cast, it will have to happen within anyway.

I've figured this out by using this little sample (with recent versions of the three compilers, targeting both x64 and ARM64):
https://godbolt.org/z/oPfdz8hTM

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

I'd suggest rebasing for good measure, just so we don't have a 5 months old object.h commit to land on during git bisect :)

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Mar 6, 2024
@akien-mga akien-mga merged commit 7307eef into godotengine:master Mar 6, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@RandomShaper RandomShaper deleted the cast_to_expl_null branch March 6, 2024 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants