-
Notifications
You must be signed in to change notification settings - Fork 407
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
Use consistent types in Kokkos_OpenMPTarget_Parallel.hpp #3703
Use consistent types in Kokkos_OpenMPTarget_Parallel.hpp #3703
Conversation
d12494c
to
b85ef20
Compare
Do you think you can come up with a compile-only test that would catch the issue? |
df94387
to
94c7c63
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good besides the Impl::
namespace issue with OpenMPTarget
|
d80a6d8
to
1ebb840
Compare
league_size) {} | ||
: m_flags( | ||
Kokkos::view_alloc(Kokkos::WithoutInitializing, "flags"), | ||
#ifdef KOKKOS_ENABLE_OPENMPTARGET |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that a FIXME?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's ask @rgayatri23 if this restriction is considered to be permanent or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully the restriction of needing at-least 32 threads per team is not permanent. We should probably add a FIXME_OPENMPTARGET everywhere we do this so that in the future if we can avoid this restriction, we can easily track these changes.
@@ -65,6 +65,8 @@ TEST(TEST_CATEGORY, team_for) { | |||
1000); | |||
} | |||
|
|||
// FIXME_OPENMPTARGET wrong results | |||
#ifndef KOKKOS_ENABLE_OPENMPTARGET | |||
TEST(TEST_CATEGORY, team_reduce) { | |||
TestTeamPolicy<TEST_EXECSPACE, | |||
Kokkos::Schedule<Kokkos::Static> >::test_reduce(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why OpenMPTarget has a different problem size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean? For me, this test just gives wrong results so I disabled it for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh Ok. I saw test_reduce(0)
and test_reduce(1000)
, so assumed that the OPENMPTARGET backend is doing something different.
LGTM |
Please cleanup history (the enable test, disable and reenable at least) and I'll approve |
18c1deb
to
299aed7
Compare
Done. |
Fixes #3702. I checked that all other backends follow this convention.