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

Test Legion use case #5206

Merged
merged 17 commits into from
Sep 28, 2022
Merged

Test Legion use case #5206

merged 17 commits into from
Sep 28, 2022

Conversation

masterleinad
Copy link
Contributor

Addressing #5144 (comment). I don't feel strongly about deleting the other test otr not.

core/unit_test/CMakeLists.txt Outdated Show resolved Hide resolved
core/unit_test/UnitTest_InitializeBackends.cpp Outdated Show resolved Hide resolved
core/unit_test/UnitTest_InitializeBackends.cpp Outdated Show resolved Hide resolved
@masterleinad masterleinad force-pushed the legion_use_case branch 2 times, most recently from beef11e to e36f8e5 Compare July 13, 2022 14:26
@masterleinad
Copy link
Contributor Author

@masterleinad
Copy link
Contributor Author

I ended up reordering execution spaces so that host spaces are initialized first since I didn't find a good way to test

#ifndef KOKKOS_IMPL_TURN_OFF_CUDA_HOST_INIT_CHECK
if (!HostSpace::execution_space::impl_is_initialized()) {
const std::string msg(
"Cuda::initialize ERROR : HostSpace::execution_space is not "
"initialized");
throw_runtime_exception(msg);
}
#endif
(since it's in a *.cc file and I didn't want to add that define to the library target).

@masterleinad masterleinad marked this pull request as ready for review July 14, 2022 21:54
@masterleinad
Copy link
Contributor Author

Only the SYCL build is timing out in Kokkos_Serial1 which looks spurious.

EXPECT_TRUE(Kokkos::Experimental::OpenMPTarget::impl_is_initialized());
#endif
#ifdef KOKKOS_ENABLE_HIP
EXPECT_FALSE(Kokkos::Experimental::HIP::impl_is_initialized());
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
EXPECT_FALSE(Kokkos::Experimental::HIP::impl_is_initialized());
EXPECT_FALSE(Kokkos::HIP::impl_is_initialized());

#endif
#ifdef KOKKOS_ENABLE_HIP
EXPECT_FALSE(Kokkos::Experimental::HIP::impl_is_initialized());
Kokkos::Experimental::HIP::impl_initialize(kokkos_init_settings);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Kokkos::Experimental::HIP::impl_initialize(kokkos_init_settings);
Kokkos::HIP::impl_initialize(kokkos_init_settings);

#ifdef KOKKOS_ENABLE_HIP
EXPECT_FALSE(Kokkos::Experimental::HIP::impl_is_initialized());
Kokkos::Experimental::HIP::impl_initialize(kokkos_init_settings);
EXPECT_TRUE(Kokkos::Experimental::HIP::impl_is_initialized());
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
EXPECT_TRUE(Kokkos::Experimental::HIP::impl_is_initialized());
EXPECT_TRUE(Kokkos::HIP::impl_is_initialized());

EXPECT_FALSE(Kokkos::Experimental::OpenMPTarget::impl_is_initialized());
#endif
#ifdef KOKKOS_ENABLE_HIP
Kokkos::Experimental::HIP::impl_finalize();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Kokkos::Experimental::HIP::impl_finalize();
Kokkos::HIP::impl_finalize();

#endif
#ifdef KOKKOS_ENABLE_HIP
Kokkos::Experimental::HIP::impl_finalize();
EXPECT_FALSE(Kokkos::Experimental::HIP::impl_is_initialized());
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
EXPECT_FALSE(Kokkos::Experimental::HIP::impl_is_initialized());
EXPECT_FALSE(Kokkos::HIP::impl_is_initialized());

@masterleinad
Copy link
Contributor Author

What I wanted to achieve with this pull request (mostly) was that the way we promised Legion to initialize Kokkos works, i.e.,
the sequence of

Kokkos::Impl::pre_initialize()
EXECUTION_SPACE::impl_initialize();
Kokkos::Impl::post_initialize();
EXECUTION_SPACE::impl_finalize();

is valid. On the way, I discovered that every backend implements EXECUTION_SPACE::impl_is_initialized() and so I used it.
If we think that this is not a useful test, I'm happy to bury it.

@crtrott
Copy link
Member

crtrott commented Sep 26, 2022

Maybe we should just use the aliases i.e. call:

if(!is_same_v<Serial, DefaultHostExecutionSpace>)
  Serial::impl_initialize();
if(!is_same_v<DefaultExecutionSpace, DefaultHostExecutionSpace>)
  DefaultHostExecutionSpace::impl_initialize();
DefaultExecutionSpace::impl_initialize(...);

Same for finalize. Then add some actual test in between.

View<int*> data("d",1000);
deep_copy(d,1);
int result;
Kokkos::parallel_reduce("TestRed", d.extent(0), KOKKOS_LAMBDA(int i, int& sum) {
  sum += d(i);
},result);
EXPECT_EQ(result, d.extent(0));

Don't use the impl_is_initialized, just check that Kokkos::is_initialized is correct.

core/unit_test/TestLegionInitialization.cpp Outdated Show resolved Hide resolved
core/unit_test/TestLegionInitialization.cpp Outdated Show resolved Hide resolved
core/unit_test/TestLegionInitialization.cpp Outdated Show resolved Hide resolved
core/unit_test/TestLegionInitialization.cpp Show resolved Hide resolved
core/unit_test/TestLegionInitialization.cpp Outdated Show resolved Hide resolved
core/src/HIP/Kokkos_HIP_Instance.cpp Outdated Show resolved Hide resolved
Copy link
Member

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

Would approve with the changes suggested in masterleinad#9

@dalg24
Copy link
Member

dalg24 commented Sep 27, 2022

I just realize the compile test is not being removed any more. Is it intentional? I am fine with the removal.

@dalg24 dalg24 merged commit 267f5eb into kokkos:develop Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants