[HIP] Implement coarse-grained memory advice for the HIP adapter #1249
Conversation
GeorgeWeb
force-pushed
the
georgi/hip_memadvise_coarse_grained
branch
from
c817d04
to
52959b8
Compare
GeorgeWeb
force-pushed
the
georgi/hip_memadvise_coarse_grained
branch
from
52959b8
to
7fdaed2
Compare
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #1249 +/- ##
==========================================
- Coverage 15.80% 15.51% -0.29%
==========================================
Files 223 238 +15
Lines 31481 33793 +2312
Branches 3558 3743 +185
==========================================
+ Hits 4975 5243 +268
- Misses 26455 28500 +2045
+ Partials 51 50 -1 ☔ View full report in Codecov by Sentry. |
…apter implementation
| @@ -92,30 +92,46 @@ ur_result_t setHipMemAdvise(const void *DevPtr, const size_t Size, | |||
| if (URAdviceFlags & | |||
| (UR_USM_ADVICE_FLAG_SET_NON_ATOMIC_MOSTLY | | |||
| UR_USM_ADVICE_FLAG_CLEAR_NON_ATOMIC_MOSTLY | | |||
| UR_USM_ADVICE_FLAG_BIAS_CACHED | UR_USM_ADVICE_FLAG_BIAS_UNCACHED)) { | |||
| UR_USM_ADVICE_FLAG_BIAS_CACHED | UR_USM_ADVICE_FLAG_BIAS_UNCACHED | |||
| #if !defined(__HIP_PLATFORM_AMD__) | |||
There was a problem hiding this comment.
Is it worth it to add ifdef conditions for this? I thought that we don't really support HIP for Nvidia devices.
There was a problem hiding this comment.
We don't actively support it but until we have completely dropped it officially there are a bunch of other entry points in the hip adapter that maintain codepaths for hip-nv platforms. Either way, I do not disagree to removing it and simplifying this function a bit this way.
…ory advise hint Co-authored-by: Fábio <fabio.m.mestre@gmail.com>
GeorgeWeb
force-pushed
the
georgi/hip_memadvise_coarse_grained
branch
from
742ea0d
to
2a9ded6
Compare
|
@oneapi-src/unified-runtime-hip-write Can I get some eyes on this one? |
| /// @cond | ||
| UR_USM_ADVICE_FLAG_FORCE_UINT32 = 0x7fffffff | ||
| /// @endcond | ||
|
|
||
| } ur_usm_advice_flag_t; | ||
| /// @brief Bit Mask for validating ur_usm_advice_flags_t | ||
| #define UR_USM_ADVICE_FLAGS_MASK 0xffff8000 | ||
| #define UR_USM_ADVICE_FLAGS_MASK 0xfffe0000 |
There was a problem hiding this comment.
This now has fewer bits set. Is this right? If so that's a somewhat unusual definition of "mask"
There was a problem hiding this comment.
This is also generated from the source generation scripts and initially threw me off too, but I think I got it now though. It is actually the inverse mask that UR uses for validation.
| @@ -6441,6 +6447,26 @@ inline ur_result_t printFlag<ur_usm_advice_flag_t>(std::ostream &os, uint32_t fl | |||
| } | |||
| os << UR_USM_ADVICE_FLAG_CLEAR_PREFERRED_LOCATION_HOST; | |||
| } | |||
|
|
|||
| if ((val & UR_USM_ADVICE_FLAG_SET_NON_COHERENT_MEMORY) == (uint32_t)UR_USM_ADVICE_FLAG_SET_NON_COHERENT_MEMORY) { | |||
There was a problem hiding this comment.
I know you're just copying from the surrounding style here, but if you're doing a bitwise AND of a single bit flag you only need to check against zero. Thus, the equality comparison and uint32_t cast is superfluous and just serves to make the line longer. Not going to block this patch, but it could do with cleaning up later, maybe
There was a problem hiding this comment.
Yeah, that could be brought up to UR because I think it was the generate cmake target that produced the code.
Enables and tests coarse grained memory access via the memadvise implementation for HIP platforms on AMD hardware. See related UR changes for the adapter implementation: oneapi-src/unified-runtime#1249 --------- Co-authored-by: Kenneth Benzie (Benie) <k.benzie@codeplay.com> Co-authored-by: aarongreig <aarongreig01@gmail.com>
Implements coarse grained memory access via a memory advice flag for HIP platforms on AMD hardware.
Related intel/llvm changes and testing: intel/llvm#12394