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

Cleanup/rework fence overloads #5148

Merged
merged 7 commits into from
Jun 27, 2022

Conversation

dalg24
Copy link
Member

@dalg24 dalg24 commented Jun 25, 2022

(Re-opening #5147 that I accidentally closed)

Yet another attempt to harmonize/simplify the interface of execution spaces.
These changes are necessary to write generic code in #5144

  • Prefer default argument in ExecutionSpace::fence and "global" fence
  • Remove ExecutionSpace::impl_static_fence(void)
  • Provide (missing) HPX::impl_static_fence
  • Drop leading execution space argument in OpenMP::impl_static_fence
  • Remove ExecSpaceInitializerBase::fence() overload that takes no label argument

@dalg24 dalg24 changed the title Prefer default argument in ExecutionSpace::[impl_static_]fence Cleanup/rework fence overloads Jun 26, 2022
@dalg24 dalg24 added the Refactor Tidying up: make code code, cleaner, simpler, understandable and robust label Jun 27, 2022
Copy link
Contributor

@masterleinad masterleinad 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 to me. Maybe, we should add some tests checking for expected labels for unnamed and static fences given some of the changes in this pull request.

@dalg24
Copy link
Member Author

dalg24 commented Jun 27, 2022

Looks good to me. Maybe, we should add some tests checking for expected labels for unnamed and static fences given some of the changes in this pull request.

What else than

/**
* Test that fencing an instance with a name yields a fence
* event of that name, and the correct device ID
*/
TEST(kokkosp, test_named_instance_fence) {
test_wrapper([&]() {
auto root = Kokkos::Tools::Experimental::device_id_root<
Kokkos::DefaultExecutionSpace>();
std::vector<FencePayload> expected{
{"named_instance", FencePayload::distinguishable_devices::no,
root + num_instances}};
expect_fence_events(expected, [=]() {
Kokkos::DefaultExecutionSpace ex;
ex.fence("named_instance");
});
num_instances += increment<Kokkos::DefaultExecutionSpace>::size;
});
}
/**
* Test that fencing an instance without a name yields a fence
* event of a correct name, and the correct device ID
*/
TEST(kokkosp, test_unnamed_instance_fence) {
test_wrapper([&]() {
auto root = Kokkos::Tools::Experimental::device_id_root<
Kokkos::DefaultExecutionSpace>();
std::vector<FencePayload> expected{
{"Unnamed Instance Fence", FencePayload::distinguishable_devices::no,
root + num_instances}};
expect_fence_events(expected, [=]() {
Kokkos::DefaultExecutionSpace ex;
ex.fence();
});
num_instances += increment<Kokkos::DefaultExecutionSpace>::size;
});
}
/**
* Test that invoking a global fence with a name yields a fence
* event of a correct name, and fences the root of the default device
*/
TEST(kokkosp, test_named_global_fence) {
test_wrapper([&]() {
auto root = Kokkos::Tools::Experimental::device_id_root<
Kokkos::DefaultExecutionSpace>();
std::vector<FencePayload> expected{
{"test global fence", FencePayload::distinguishable_devices::no, root}};
expect_fence_events(expected,
[=]() { Kokkos::fence("test global fence"); });
});
}
/**
* Test that invoking a global fence with no name yields a fence
* event of a correct name, and fences the root of the default device
*/
TEST(kokkosp, test_unnamed_global_fence) {
test_wrapper([&]() {
auto root = Kokkos::Tools::Experimental::device_id_root<
Kokkos::DefaultExecutionSpace>();
std::vector<FencePayload> expected{
{"Unnamed Global Fence", FencePayload::distinguishable_devices::no,
root}};
expect_fence_events(expected, [=]() { Kokkos::fence(); });
num_instances += increment<Kokkos::DefaultExecutionSpace>::size;
});
would you like to see?

Co-Authored-By: Daniel Arndt <arndtd@ornl.gov>
@dalg24
Copy link
Member Author

dalg24 commented Jun 27, 2022

Ignoring OpenMPTarget hang that will likely timeout

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Refactor Tidying up: make code code, cleaner, simpler, understandable and robust
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants