Skip to content

Conversation

@hjelmn
Copy link
Member

@hjelmn hjelmn commented Jan 25, 2021

Benchmarks are showing better performance when not using the __atomic
built-ins on this arch. This PR disables them by default for this
architecture only.

Signed-off-by: Nathan Hjelm [email protected]

@hjelmn hjelmn added this to the v4.1.1 milestone Jan 25, 2021
@hjelmn hjelmn requested a review from bosilca January 25, 2021 16:37
@hjelmn hjelmn force-pushed the v4.1.x_use_hand_written_atomics_for_aarch64_by_default_because_the_builtins_are_inferior_for_our_usage branch from 3564f1a to 37b7d87 Compare January 25, 2021 16:38
@rajachan
Copy link
Member

FWIW, this had come up just before the 4.1.0 release. We observe this on one of our aarch64 instances as well. @jsquyres had some thoughts on changing the default, tagging him for review.

@rajachan rajachan requested a review from jsquyres January 25, 2021 16:41
@hjelmn
Copy link
Member Author

hjelmn commented Jan 25, 2021

@rajachan Yeah. I think now that AArch64 is getting into more hands there is good reason to push this to v4.1.x and v4.0.x. We want to provide the best out of the box performance on Apple Silicon.

@jsquyres
Copy link
Member

Let's put a bullet in NEWS since we're changing it past 4.1.0. But this is such an inner-magic change that I don't think anyone will notice (other than better performance).

@hjelmn
Copy link
Member Author

hjelmn commented Jan 25, 2021

@jsquyres Ok. Will do that now.

Benchmarks are showing better performance when not using the __atomic
built-ins on this arch. This PR disables them by default for this
architecture only.

Signed-off-by: Nathan Hjelm <[email protected]>
@hjelmn hjelmn force-pushed the v4.1.x_use_hand_written_atomics_for_aarch64_by_default_because_the_builtins_are_inferior_for_our_usage branch from 37b7d87 to 3248a77 Compare January 25, 2021 16:55
@hjelmn
Copy link
Member Author

hjelmn commented Jan 25, 2021

Done.

@rajachan rajachan merged commit 16f4797 into open-mpi:v4.1.x Jan 25, 2021
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.

4 participants