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

enable Half in mpi #1759

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
Open

enable Half in mpi #1759

wants to merge 10 commits into from

Conversation

yhmtsai
Copy link
Member

@yhmtsai yhmtsai commented Dec 30, 2024

This PR enables half precision in distributed environment by adding custom operation.

one-side operation like accumulation and fetch_and_op does not support custom operation.

Note. Newer version of mpi might support half precision natively (also for one-side operation) if the administrator build it with compiler supporting native half precision and enable the option.

TODO:

  • enable the rest distributed function with half
  • put the custom operation in gko::comm? it does not grow along with #nodes -> create/free when necessary

@yhmtsai yhmtsai added the 1:ST:WIP This PR is a work in progress. Not ready for review. label Dec 30, 2024
@yhmtsai yhmtsai self-assigned this Dec 30, 2024
@ginkgo-bot ginkgo-bot added reg:testing This is related to testing. type:solver This is related to the solvers type:preconditioner This is related to the preconditioners mod:all This touches all Ginkgo modules. labels Dec 30, 2024
@yhmtsai yhmtsai added 1:ST:ready-for-review This PR is ready for review and removed 1:ST:WIP This PR is a work in progress. Not ready for review. labels Jan 2, 2025
@yhmtsai yhmtsai requested a review from a team January 2, 2025 08:30
@MarcelKoch MarcelKoch self-requested a review January 7, 2025 08:03
Copy link
Member

@MarcelKoch MarcelKoch left a comment

Choose a reason for hiding this comment

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

I'm mainly concerned about using device buffers for the custom operations, and maybe moving the operations into a private header.



template <typename ValueType>
inline void sum(void* input, void* output, int* len, MPI_Datatype* datatype)
Copy link
Member

Choose a reason for hiding this comment

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

Do these functions work with device buffers? Or is it maybe necessary to copy the device buffers first to the host and then do the operation?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have the same concern.
However, I think mpi will not use device memory to handle the reduction operation. Even in frontier, they have additional unit to handle not use gpu to sum cross nodes.

Comment on lines 665 to 666
mpi::op_manager sum_op_;
mpi::op_manager norm_sum_op_;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the vector needs to store these. We can either just create them when needed, and delete them directly afterward. There should not be a large overhead associated to that. Or we use globals (probably in the form of returning a static variable). I would not mind using that, since we only need to have one operation per value type anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have tried the static variable already but it did not work out.
We call MPI_finalize to finish, but the static variable will be destroyed when the process leave main scope, which is after MPI_finalize and MPI complains that.

Copy link
Member

Choose a reason for hiding this comment

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

I tried some simple benchmarking of creating/freeing the MPI_Op, and it seems to not scale with the number of processes. I guess that is to be expected, as it is not a collective operation. So I would suggest to just create the ops on-the-fly when necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks for profiling! just to be sure, you mean the runtime is not scale with the number of processes, right?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it has a fixed overhead that doesn't depend on the number of processes.

include/ginkgo/core/base/mpi.hpp Outdated Show resolved Hide resolved
include/ginkgo/core/distributed/vector.hpp Outdated Show resolved Hide resolved

static std::unique_ptr<target_type> create_empty(const source_type* source)
{
return target_type::create(source->get_executor(),
source->get_communicator());
}

// Allow to create_empty of the same type
// For distributed case, next<next<V>> will be V in the candicated list.
Copy link
Member

Choose a reason for hiding this comment

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

You mean if half is disabled, right? But shouldn't that also lead to issues with Dense?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes.
I do not think so because Dense will use

static std::unique_ptr<TargetType> create_empty(const SourceType* source)
to call create(exec) function and Dense can convert_to itself, which satisfies the enable_if condition.

@yhmtsai yhmtsai requested a review from MarcelKoch January 15, 2025 16:53
Copy link
Member

@MarcelKoch MarcelKoch left a comment

Choose a reason for hiding this comment

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

I would suggest removing the heap allocation for predefined mpi ops, rest looks good.

} // namespace detail


using op_manager = std::shared_ptr<MPI_Op>;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just store the MPI_Op in a struct, so that you don't need to allocate/free anything for predefined MPI_Ops.
You could also keep the unique_ptr for the custom op, and only use the struct for the predefined ones.

@@ -396,6 +396,9 @@ using TwoValueIndexTypes = add_to_cartesian_type_product_t<
using ValueLocalGlobalIndexTypesBase =
Copy link
Member

Choose a reason for hiding this comment

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

this is now unused.

@@ -90,7 +90,7 @@ void count_non_owning_entries(
num_parts, local_part, row_part_ptrs.get_data(), send_count.get_data());
}

GKO_INSTANTIATE_FOR_EACH_VALUE_AND_LOCAL_GLOBAL_INDEX_TYPE_BASE(
GKO_INSTANTIATE_FOR_EACH_VALUE_AND_LOCAL_GLOBAL_INDEX_TYPE(
Copy link
Member

Choose a reason for hiding this comment

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

The base version of that macro is now unused.

@pratikvn pratikvn self-requested a review January 17, 2025 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1:ST:ready-for-review This PR is ready for review 1:ST:run-full-test mod:all This touches all Ginkgo modules. reg:testing This is related to testing. type:preconditioner This is related to the preconditioners type:solver This is related to the solvers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants