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

Type traits #206

Merged
merged 4 commits into from
Aug 4, 2023
Merged

Type traits #206

merged 4 commits into from
Aug 4, 2023

Conversation

ndryden
Copy link
Collaborator

@ndryden ndryden commented Jul 26, 2023

This exposes type traits describing Aluminum communication operations. Previously they were in the test utilities and not so well-organized.

@ndryden ndryden added the enhancement New feature or request label Jul 26, 2023
@ndryden ndryden requested a review from benson31 July 26, 2023 20:40
Copy link
Collaborator

@benson31 benson31 left a comment

Choose a reason for hiding this comment

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

This looks good. I don't think this is missing anything that Hydrogen needs, but if I'm wrong, I can PR as needed.

- Removed `AlBackendName` from op_dispatcher.h that I forgot to remove
and updated uses.
- Moved some algorithm description into a separate file to cut down on
circular includes.
@ndryden ndryden requested a review from benson31 July 31, 2023 17:16
@ndryden
Copy link
Collaborator Author

ndryden commented Jul 31, 2023

@benson31 Rerequesting review because I moved some more stuff around.

Comment on lines +204 to +209
#ifdef AL_HAS_HALF
template <> struct IsTypeSupportedByMPI<__half> : std::true_type {};
#endif
#ifdef AL_HAS_BFLOAT
template <> struct IsTypeSupportedByMPI<al_bfloat16> : std::true_type {};
#endif
Copy link
Collaborator

@benson31 benson31 Aug 1, 2023

Choose a reason for hiding this comment

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

Is this true? There's no MPI_Datatype value for these types and one cannot, say, MPI_Reduce with them (correctly) without a custom reduction function, so it seems to me that either these should be std::false_type or the documentation should be clarified as to what "supported specifically by MPI itself" means.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. I'll clarify the documentation that these are supported via Aluminum magic. (Specifically: The TypeMap that everything uses will map them to a dummy datatype, and a custom reduction operator will be used.)

Copy link
Collaborator

@benson31 benson31 left a comment

Choose a reason for hiding this comment

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

Assuming the only additional changes are the documentation update discussed in other comments, this looks good to me.

@ndryden ndryden merged commit 9e34c93 into master Aug 4, 2023
@ndryden ndryden deleted the traits branch August 4, 2023 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants