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

Avoid using execution_space as View template argument #4675

Merged
merged 3 commits into from
Jan 25, 2022
Merged

Avoid using execution_space as View template argument #4675

merged 3 commits into from
Jan 25, 2022

Conversation

masterleinad
Copy link
Contributor

Should also fix #3475.

@masterleinad masterleinad linked an issue Jan 14, 2022 that may be closed by this pull request
Can't specify memory space for internal Bitset in UnorderedMap construction #3475
Closed
@masterleinad
Copy link
Contributor Author

Retest this please.

@dalg24
Copy link
Member

dalg24 commented Jan 19, 2022

Is this still a work in progress or is it ready for review?

@masterleinad masterleinad marked this pull request as ready for review January 19, 2022 14:53
@masterleinad
Copy link
Contributor Author

Is this still a work in progress or is it ready for review?

Seems like it worked immediately. 🙂

dalg24
dalg24 reviewed Jan 19, 2022
View reviewed changes
algorithms/src/Kokkos_Random.hpp Outdated
@@ -892,7 +892,7 @@ template <class DeviceType = Kokkos::DefaultExecutionSpace>
class Random_XorShift64_Pool {
private:
using execution_space = typename DeviceType::execution_space;
using locks_type = View<int**, execution_space>;
using locks_type = View<int**, typename DeviceType::memory_space>;
Copy link
Member

Choose a reason for hiding this comment

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

Why not jut

Suggested change
using locks_type = View<int**, typename DeviceType::memory_space>;
using locks_type = View<int**, DeviceType>;

containers/src/Kokkos_Bitset.hpp Outdated
@@ -302,7 +301,8 @@ class Bitset {
private:
unsigned m_size;
unsigned m_last_block_mask;
View<unsigned*, execution_space, MemoryTraits<RandomAccess> > m_blocks;
View<unsigned*, typename Device::memory_space, MemoryTraits<RandomAccess> >
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
View<unsigned*, typename Device::memory_space, MemoryTraits<RandomAccess> >
View<unsigned*, Device, MemoryTraits<RandomAccess> >

containers/src/Kokkos_Bitset.hpp Outdated
@@ -383,7 +383,9 @@ class ConstBitset {

private:
unsigned m_size;
View<const unsigned*, execution_space, MemoryTraits<RandomAccess> > m_blocks;
View<const unsigned*, typename Device::memory_space,
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
View<const unsigned*, typename Device::memory_space,
View<const unsigned*, Device,

core/src/impl/Kokkos_ViewArray.hpp Outdated
@@ -355,7 +355,9 @@ class ViewMapping<Traits, Kokkos::Array<>> {

using execution_space = typename alloc_prop::execution_space;
using memory_space = typename Traits::memory_space;
using functor_type = ViewValueFunctor<execution_space, scalar_type>;
using functor_type =
ViewValueFunctor<Kokkos::Device<execution_space, memory_space>,
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
ViewValueFunctor<Kokkos::Device<execution_space, memory_space>,
ViewValueFunctor<typename Traits::device_type,

Copy link
Member

Choose a reason for hiding this comment

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

PIng

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See 820c00c.

@dalg24
Copy link
Member

dalg24 commented Jan 19, 2022 •
edited

Also this is not adding any test. How did you verify it resolves the issue reported?

@masterleinad
Copy link
Contributor Author

masterleinad commented Jan 19, 2022 •
edited

Also this is not adding any test. How did you verify it resolves the issue reported?

I checked manually compiling with UVM and then setting the memory_space for UnorderedMap to CudaSpace in the UnorderedMap unit tests that we don't allocate in UVM space using KokkosTools' MemoryEvents . I didn't check anything with respect to the other changes.

I'm not quite sure if we require a test here.

@crtrott crtrott added the Blocks Promotion Overview issue for release-blocking bugs label Jan 19, 2022
@nmm0 nmm0 added this to In progress in Kokkos Release 3.6 via automation Jan 19, 2022
crtrott
crtrott requested changes Jan 24, 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.

After thinking it more over I agree that DeviceType is better as the type than DeviceType::memory_space, it actually is closer to the original intend of using the optimal execution memory space for the chosen execution space.

crtrott
crtrott approved these changes Jan 25, 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.

After thinking it more over I agree that DeviceType is better as the type than DeviceType::memory_space, it actually is closer to the original intend of using the optimal execution memory space for the chosen execution space.

@dalg24 dalg24 merged commit 2d2087f into kokkos:develop Jan 25, 2022
Kokkos Release 3.6 automation moved this from In progress to Done Jan 25, 2022
@ndellingwood ndellingwood mentioned this pull request Jan 26, 2022
Closed
masterleinad added a commit to masterleinad/kokkos that referenced this pull request Feb 2, 2022
@masterleinad
Avoid using execution_space as View template argument (kokkos#4675)
c1d9b8c 
* Avoid using execution_space as View template argument

* Prefer device type over memory space type as View template parameter

* Fix one more
@masterleinad masterleinad mentioned this pull request Feb 9, 2022
Closed
@dalg24 dalg24 removed the Blocks Promotion Overview issue for release-blocking bugs label Feb 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dalg24 dalg24 dalg24 left review comments

@crtrott crtrott crtrott approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Kokkos Release 3.6
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Can't specify memory space for internal Bitset in UnorderedMap construction
3 participants
@masterleinad @dalg24 @crtrott