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.

Sign up for GitHub

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

Jump to bottom

SharedHostPinnedSpace alias in fwd declaration #5405

Merged
merged 10 commits into from
Oct 4, 2022
Merged

SharedHostPinnedSpace alias in fwd declaration #5405

merged 10 commits into from
Oct 4, 2022

Conversation

JBludau
Copy link
Contributor

@JBludau JBludau commented Aug 29, 2022 •
edited

Will need a rebase after #5289 is merged -> done

At the moment there is no SharedHostPinnedSpace for OpenMPTarget and OpenACC.
The unittest checks the existance of the alias and asserts that the changes made in one ExecutionSpace are visible in the other.
Some of the examples/tutorials could be changed to use the new SharedSpace and SharedHostPinnedSpace -> separate pr

crtrott
crtrott requested changes Sep 26, 2022
View reviewed changes
Copy link
Member

@crtrott crtrott left a comment

Choose a reason for hiding this comment

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

Needs the "does this exist" macro and constexpr bool.

JBludau
JBludau commented Sep 26, 2022
View reviewed changes
core/src/Kokkos_Core_fwd.hpp
using SharedSpace = HostSpace;
using SharedSpace = HostSpace;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure what clang-format is doing here ...

core/src/Kokkos_Core_fwd.hpp
Comment on lines +214 to +221
using SharedHostPinnedSpace = HIPHostPinnedSpace;
#define KOKKOS_HAS_SHARED_HOST_PINNED_SPACE 1
#elif defined(KOKKOS_ENABLE_SYCL)
using SharedHostPinnedSpace = Experimental::SYCLHostUSMSpace;
#define KOKKOS_HAS_SHARED_HOST_PINNED_SPACE 1
#elif !defined(KOKKOS_ENABLE_OPENACC) && !defined(KOKKOS_ENABLE_OPENMPTARGET)
using SharedHostPinnedSpace = HostSpace;
#define KOKKOS_HAS_SHARED_HOST_PINNED_SPACE 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

... and here ...

@JBludau
Copy link
Contributor Author

JBludau commented Sep 26, 2022

Needs the "does this exist" macro and constexpr bool.

is added in 5ae6d4

@JBludau JBludau marked this pull request as ready for review September 26, 2022 19:30
dalg24
dalg24 reviewed Sep 26, 2022
View reviewed changes
core/unit_test/CMakeLists.txt Outdated Show resolved Hide resolved
core/unit_test/TestSharedHostPinnedSpace.cpp Outdated Show resolved Hide resolved
core/unit_test/TestSharedHostPinnedSpace.cpp Outdated Show resolved Hide resolved
core/unit_test/TestSharedHostPinnedSpace.cpp Outdated Show resolved Hide resolved
core/unit_test/TestSharedHostPinnedSpace.cpp Outdated Show resolved Hide resolved
core/unit_test/TestSharedHostPinnedSpace.cpp Outdated Show resolved Hide resolved
core/unit_test/TestSharedHostPinnedSpace.cpp Outdated Show resolved Hide resolved
core/unit_test/TestSharedHostPinnedSpace.cpp Outdated Show resolved Hide resolved
JBludau and others added 2 commits September 27, 2022 14:28
@JBludau
moved ExecutionSpace template to ctor
f7ee93a
@JBludau @dalg24
moved assert into main loop
d978632 
Co-authored-by: Damien L-G <dalg24+github@gmail.com>
dalg24
dalg24 reviewed Sep 27, 2022
View reviewed changes
core/unit_test/TestSharedHostPinnedSpace.cpp Outdated Show resolved Hide resolved
core/unit_test/TestSharedHostPinnedSpace.cpp Outdated Show resolved Hide resolved
core/unit_test/TestSharedHostPinnedSpace.cpp Outdated Show resolved Hide resolved
core/unit_test/TestSharedHostPinnedSpace.cpp Outdated Show resolved Hide resolved
core/unit_test/TestSharedHostPinnedSpace.cpp Outdated Show resolved Hide resolved
core/unit_test/TestSharedHostPinnedSpace.cpp Outdated Show resolved Hide resolved
@JBludau @dalg24
Update core/unit_test/TestSharedHostPinnedSpace.cpp
a30c2bf 
Co-authored-by: Damien L-G <dalg24+github@gmail.com>
masterleinad
masterleinad reviewed Sep 28, 2022
View reviewed changes
core/src/Kokkos_Core_fwd.hpp
@@ -196,7 +196,7 @@ using SharedSpace = Experimental::SYCLSharedUSMSpace;
#define KOKKOS_HAS_SHARED_SPACE 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you remind me why we gave this a value instead of using a simple

#define KOKKOS_HAS_SHARED_SPACE

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my reasoning was to allow using it inside an if ... even though we have the constexp var

https://godbolt.org/z/TdoWfrMoG

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 like it but this was already there. Maybe we can discuss at the developer meeting today and possibly update in a follow up PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good.

my reasoning was to allow using it inside an if ... even though we have the constexp var godbolt.org/z/TdoWfrMoG

In that case, I would have expected that the macro is defined with value 0 if the feature is not available.

JBludau and others added 2 commits September 28, 2022 03:39
@JBludau @dalg24
changed to camelCase and numErrors
593cb45 
Co-authored-by: Damien L-G <dalg24+github@gmail.com>
@JBludau @dalg24
addressed dalg24 review comments
0fac2a5 
Co-authored-by: Damien L-G <dalg24+github@gmail.com>
dalg24
dalg24 approved these changes Sep 28, 2022
View reviewed changes
core/unit_test/TestSharedHostPinnedSpace.cpp Outdated
Kokkos::RangePolicy<ExecutionSpace, Kokkos::IndexType<size_t>>{
0, view_.size()},
*this, Kokkos::Sum<unsigned>(numErrors));
Kokkos::fence();
Copy link
Member

Choose a reason for hiding this comment

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

Why did you add a fence by the way?

Copy link
Member

Choose a reason for hiding this comment

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

That fence is not needed.

Copy link
Member

Choose a reason for hiding this comment

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

None of the two fences are

@crtrott
Copy link
Member

crtrott commented Sep 29, 2022

Retest this please.

@masterleinad
Copy link
Contributor

CUDA-10.1-Clang-Tidy shows:

[ RUN      ] defaultdevicetype.shared_host_pinned_space
11: /var/jenkins/workspace/Kokkos/core/unit_test/TestSharedHostPinnedSpace.cpp:115: Failure
11: Expected equality of these values:
11:   CheckResult(HostExecutionSpace{}, sharedData, incrementCount).numErrors
11:     Which is: 1024
11:   0u
11:     Which is: 0

This was referenced Oct 1, 2022
Merged
Merged
@JBludau
Copy link
Contributor Author

JBludau commented Oct 3, 2022

Retest this please

1 similar comment
@JBludau
Copy link
Contributor Author

JBludau commented Oct 3, 2022

Retest this please

@dalg24 dalg24 merged commit aa98af2 into kokkos:develop Oct 4, 2022
@JBludau JBludau deleted the SharedHostPinnedSpace branch October 4, 2022 21:00
This was referenced Oct 5, 2022
Closed
Closed
@BenWibking BenWibking mentioned this pull request May 4, 2023
Open
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dalg24 dalg24 dalg24 approved these changes

@crtrott crtrott crtrott approved these changes

@masterleinad masterleinad masterleinad approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@JBludau @crtrott @masterleinad @dalg24