Full GPU support for axom::Array by joshessman-llnl · Pull Request #695 · llnl/axom

joshessman-llnl · 2021-10-27T13:04:36Z

Summary

This PR is a feature
It does the following:
- Extends Array and in particular ArrayView to run on the GPU
- Adds a MemorySpace parameter to allow users to optionally lock down the memory space of an Array at compile-time

Remaining tasks (non-exhaustive):

Multidim indexing/tests
Documentation updates

New docs:
https://axom.readthedocs.io/en/feature-essman-gpu_array/axom/core/docs/sphinx/core_containers.html#using-arrays-in-gpu-code

agcapps

This is very good, Josh. I had a few questions, one of which (mismatch of memory space template parameter with constructor-specified allocator) may need more documentation or testing. I think I'm ready to approve once you've clarified those things.

agcapps · 2021-11-02T18:32:56Z

src/axom/core/ArrayBase.hpp

+ * This abstraction is not implemented using Umpire's MemoryResourceType enum
+ * in order to also include a "Dynamic" option as a default template parameter
+ * for Array-like types


Thank you for this note!

src/axom/core/docs/sphinx/core_containers.rst

src/axom/core/examples/core_containers.cpp

src/axom/core/docs/sphinx/core_containers.rst

agcapps · 2021-11-03T17:52:56Z

And thank you for the link to the branch's docs on readthedocs. That is very helpful when reviewing documentation.

Co-authored-by: Arlie Capps <48997041+agcapps@users.noreply.github.com>

src/axom/core/Array.hpp

publixsubfan · 2021-11-03T23:59:47Z

src/axom/core/Array.hpp

   * \brief Exchanges the contents of this Array with the other.
   */
-  void swap(Array<T, DIM>& other);
+  void swap(Array<T, DIM, SPACE>& other);


Can this be extended to accept Array other from other memory spaces as well?

I can add a swap function between memory spaces but I'd be curious to hear what some use cases might be for this - in the general case you'd have to copy both sets of data between memory spaces, so I think the only thing you'd really be swapping are the housekeeping/metadata variables.

It might be useful for memory spaces that are compatible.
E.g. a Dynamic space that happens to be device memory (via umpire) and a Device space ?

publixsubfan · 2021-11-04T00:01:00Z

src/axom/core/Array.hpp

+  : ArrayBase<T, DIM, Array<T, DIM, SPACE>>(
+      static_cast<const ArrayBase<T, DIM, Array<T, DIM, SPACE>>&>(other))
+  , m_allocator_id(SPACE == MemorySpace::Dynamic
+                     ? allocator_id


What if allocator_id is from the correct memory space, but a different pool?

I'm not an Umpire expert by any means but I believe in this case we'd have to do some introspection on the allocator to try to track down its "parent allocator"?

src/axom/core/ArrayBase.hpp

kennyweiss

Thanks @joshessman-llnl -- this is really nice and highly anticipated!

Please see my comments for some minor suggestions/corrections.

src/axom/core/ArrayBase.hpp

kennyweiss · 2021-11-04T22:35:00Z

src/axom/core/ArrayView.hpp

+#ifdef AXOM_USE_UMPIRE
+  // If we have Umpire, we can try and see what space the pointer is allocated in
+  // Probably not worth checking this if SPACE != Dynamic, we *could* error out
+  // if e.g., the user gives a host pointer to ArrayView<T, DIM, Device>, but even
+  // Thrust doesn't guard against this.
+
+  // FIXME: Is it worth trying to get rid of this at compile time?
+  // (using a workaround since we don't have "if constexpr")
+  if(SPACE == MemorySpace::Dynamic)
+  {
+    auto& rm = umpire::ResourceManager::getInstance();
+    if(rm.hasAllocator(data))
+    {
+      auto alloc = rm.getAllocator(data);
+      m_allocator_id = alloc.getId();
+    }
+  }
+#endif


Possible suggestion: Would it make sense to fold this into the getAllocatorID function for <SPACE == Dynamic> ?

I'm probably missing something but I'm not clear on how getAllocatorID comes into play here - were you envisioning an overload getAllocatorID(T* ptr) that encapsulates the pointer introspection?

src/axom/core/StackArray.hpp

kennyweiss · 2021-11-04T22:39:44Z

src/axom/core/examples/core_containers.cpp

  // _iteration_end
 }

+#ifdef __CUDACC__


Heads-up @bmhan12 -- we'll want to extend this example to support hip

src/axom/core/docs/sphinx/core_containers.rst

kennyweiss · 2021-11-04T23:32:07Z

src/axom/core/ArrayBase.hpp

+/// \brief Translates between the MemorySpace enum and Umpire allocator IDs
+template <MemorySpace SPACE>
+inline int getAllocatorID();
+


Minor (optional): It might be useful to have a utility function of the form:

bool areSpacesCompatible(MemorySpace space1, MemorySpace space2);

And possibly also:

#ifdef AXOM_USE_UMPIRE bool areSpacesCompatible(MemorySpace SPACE, int allocatorID); #endif

I can understand the utility of the second function (in particular, relating to @kanye-quest's comment about pooled allocators and also converting/swapping from Dynamic -> another space) but I'm not sure where/how the first one would be used.

src/axom/core/examples/core_containers.cpp

src/axom/core/docs/sphinx/core_containers.rst

src/axom/core/ArrayView.hpp

…ons to ArrayView

src/axom/core/ArrayBase.hpp

white238 · 2021-11-08T20:59:46Z

src/axom/core/examples/CMakeLists.txt

-      blt_add_executable(
-          NAME        ${exe_name}_cuda_on_ex
+    blt_add_executable(
+          NAME        core_acceleration_cuda_on_ex


Good catch...

joshessman-llnl added 6 commits October 26, 2021 08:53

feat: add memoryspace template param to Array

69bc053

fix: typo in unified memory test, set capacity == size

0539d42

tests: add initial tests of Array on a device

054dea7

feat: device-compatible ArrayView

e2c6d0c

fix: move type trait out of ifdef

f84b7da

Merge branch 'develop' into feature/essman/gpu_array

254a5b6

joshessman-llnl added the Core Issues related to Axom's 'core' component label Oct 27, 2021

joshessman-llnl mentioned this pull request Oct 27, 2021

Array Updates #633

Open

20 tasks

joshessman-llnl added 4 commits October 27, 2021 10:27

fix: try to dodge MSVC compiler bug

4c45008

refactor: add support (and tests) for multidimensional indexing on GPU

6aaf7ad

fix: const overloads for StackArray iterators

93a4dab

fix: yet another msvc fix

21bdb08

publixsubfan mentioned this pull request Oct 28, 2021

ImplicitGrid port to GPU #677

Merged

7 tasks

joshessman-llnl and others added 4 commits November 1, 2021 09:55

Merge branch 'develop' into feature/essman/gpu_array

3eafde7

examples: add examples of axom::Array usage in device code

b42ebad

docs: document new example code

724be26

docs: final cleanup

6644fd6

joshessman-llnl changed the title ~~WIP: Full GPU support for axom::Array~~ Full GPU support for axom::Array Nov 2, 2021

joshessman-llnl requested review from agcapps, bmhan12, kennyweiss, publixsubfan and white238 November 2, 2021 16:23

agcapps reviewed Nov 3, 2021

View reviewed changes

docs: apply suggestions from @agcapps

3c0b3e1

Co-authored-by: Arlie Capps <48997041+agcapps@users.noreply.github.com>

publixsubfan reviewed Nov 3, 2021

View reviewed changes

src/axom/core/Array.hpp Outdated Show resolved Hide resolved

publixsubfan reviewed Nov 3, 2021

View reviewed changes

src/axom/core/Array.hpp Show resolved Hide resolved

publixsubfan reviewed Nov 3, 2021

View reviewed changes

publixsubfan reviewed Nov 4, 2021

View reviewed changes

src/axom/core/ArrayBase.hpp Outdated Show resolved Hide resolved

cleanup: apply feedback regarding SFINAE and docs

a61552c

kennyweiss approved these changes Nov 4, 2021

View reviewed changes

joshessman-llnl added 3 commits November 8, 2021 08:19

Merge branch 'develop' into feature/essman/gpu_array

79553eb

cleanup: incorporate suggestions from @kennyweiss

a6e9bee

docs: add simplified example demonstrating implicit/explicit conversi…

23dee67

…ons to ArrayView

white238 reviewed Nov 8, 2021

View reviewed changes

src/axom/core/ArrayBase.hpp Outdated Show resolved Hide resolved

rhornung67 added this to the Feb 2022 Release milestone Nov 8, 2021

fix: update docstring for type trait

f39b02a

white238 approved these changes Nov 9, 2021

View reviewed changes

joshessman-llnl and others added 2 commits November 9, 2021 03:16

Merge branch 'develop' into feature/essman/gpu_array

853537c

docs: mostly no-op to retrigger CI

37f6409

joshessman-llnl force-pushed the feature/essman/gpu_array branch from 21c5f45 to 37f6409 Compare November 9, 2021 13:16

joshessman-llnl merged commit 3999cf2 into develop Nov 9, 2021

joshessman-llnl deleted the feature/essman/gpu_array branch November 9, 2021 14:02

Conversation

joshessman-llnl commented Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

agcapps left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

agcapps commented Nov 3, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kennyweiss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kennyweiss Nov 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshessman-llnl Nov 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

joshessman-llnl commented Oct 27, 2021 •

edited

Loading

kennyweiss Nov 4, 2021 •

edited

Loading

joshessman-llnl Nov 8, 2021 •

edited

Loading