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

HIP updates: managed_memory_resource and missing header #272

Merged
merged 5 commits into from
Mar 7, 2024

Conversation

StewMH
Copy link
Contributor

@StewMH StewMH commented Mar 6, 2024

Fixes compilation in the traccc alpaka HIP build.

@StewMH StewMH changed the title Add missing include HIP updates: managed_memory_resource and missing header Mar 6, 2024
Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

I was quite mystified as to why the unit tests of this project would not have shown this issue before. But then I found:

https://github.com/acts-project/vecmem/blob/main/tests/hip/test_hip_containers_kernels.hip#L8-L10

In the test where we exercise vecmem::device_atomic_ref in HIP device code, we already include <hip/hip_runtime.h> to make assert(...) work. So the code was/is blind to device_atomic_ref.h needing this include as well. 😦

Was HIP managed memory introduced with ROCm 5? I remember having looked for it back when I first wrote this code with ROCm 4. But I didn't find such functionality back then. 🤔 Or couse ROCm 4 is not even installed on any of my machines anymore, so I couldn't even double-check just now if this was indeed the case or not. 😕

You should fix the formatting error(s) flagged by the CI, update the copyright statements, and this is good to go from my side. 👍

@krasznaa
Copy link
Member

krasznaa commented Mar 7, 2024

Actually... 🤔 Could you please do this?

diff --git a/core/include/vecmem/memory/impl/atomic.ipp b/core/include/vecmem/memory/impl/atomic.ipp
index 1aebc28..0008511 100644
--- a/core/include/vecmem/memory/impl/atomic.ipp
+++ b/core/include/vecmem/memory/impl/atomic.ipp
@@ -1,12 +1,17 @@
 /*
  * VecMem project, part of the ACTS project (R&D line)
  *
- * (c) 2021 CERN for the benefit of the ACTS project
+ * (c) 2021-2024 CERN for the benefit of the ACTS project
  *
  * Mozilla Public License Version 2.0
  */
 #pragma once
 
+// HIP include(s).
+#if defined(__HIP_DEVICE_COMPILE__)
+#include <hip/hip_runtime.h>
+#endif
+
 // SYCL include(s).
 #if defined(CL_SYCL_LANGUAGE_VERSION) || defined(SYCL_LANGUAGE_VERSION)
 #include <CL/sycl.hpp>
diff --git a/core/include/vecmem/memory/impl/device_atomic_ref.ipp b/core/include/vecmem/memory/impl/device_atomic_ref.ipp
index 021c648..0b3fe7e 100644
--- a/core/include/vecmem/memory/impl/device_atomic_ref.ipp
+++ b/core/include/vecmem/memory/impl/device_atomic_ref.ipp
@@ -1,12 +1,17 @@
 /*
  * VecMem project, part of the ACTS project (R&D line)
  *
- * (c) 2022-2023 CERN for the benefit of the ACTS project
+ * (c) 2022-2024 CERN for the benefit of the ACTS project
  *
  * Mozilla Public License Version 2.0
  */
 #pragma once
 
+// HIP include(s).
+#if defined(__HIP_DEVICE_COMPILE__)
+#include <hip/hip_runtime.h>
+#endif
+
 // SYCL include(s).
 #if defined(CL_SYCL_LANGUAGE_VERSION) || defined(SYCL_LANGUAGE_VERSION)
 #include <CL/sycl.hpp>
diff --git a/tests/hip/test_hip_containers_kernels.hip b/tests/hip/test_hip_containers_kernels.hip
index e3240e8..5cb83df 100644
--- a/tests/hip/test_hip_containers_kernels.hip
+++ b/tests/hip/test_hip_containers_kernels.hip
@@ -1,14 +1,10 @@
 /* VecMem project, part of the ACTS project (R&D line)
  *
- * (c) 2021-2023 CERN for the benefit of the ACTS project
+ * (c) 2021-2024 CERN for the benefit of the ACTS project
  *
  * Mozilla Public License Version 2.0
  */
 
-// HIP include(s). Note that this needs to come first, as it affects how
-// other headers later on include/see system headers like <cassert>.
-#include <hip/hip_runtime.h>
-
 // Local include(s).
 #include "../../hip/src/utils/hip_error_handling.hpp"
 #include "test_hip_containers_kernels.hpp"
@@ -21,6 +17,9 @@
 #include "vecmem/memory/device_atomic_ref.hpp"
 #include "vecmem/utils/tuple.hpp"
 
+// HIP include(s).
+#include <hip/hip_runtime.h>
+
 /// Kernel performing a linear transformation using the vector helper types
 __global__ void linearTransformKernel(
     vecmem::data::vector_view<const int> constants,

We should make our tests more sensitive to such errors. And also fix atomic.ipp. 😉

@StewMH
Copy link
Contributor Author

StewMH commented Mar 7, 2024

Thanks Attila. I couldn't get your patch to apply but I've hopefully done something similar.

Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

After a small additional tweak, I'm happy with this.

@krasznaa krasznaa merged commit 5f2d20e into acts-project:main Mar 7, 2024
22 checks passed
@StewMH StewMH deleted the hip_include_device branch April 11, 2024 10:24
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.

2 participants