[HIP] Explicitly specify copy direction for USM 2D async memory copies

[GeorgeWeb]

There is an issue with hipMemcpy2D* when hipMemcpyDefault is used, which makes the HIP runtime not correctly derive the copy kind (direction) for the copies introduced since ROCm 5.6.0. See: ROCm/clr#40.

This PR aims to provide a work-around in the meantime even though having to introduce the overhead of hipPointerGetAttributes in the memcpy2D. It patches it to work for the all possible copy kind directions (host / device / os-allocated host pageable memory) explicitly. Ideally, ROCm will fix the API when using just hipMemcpyDefault going forward.

Testing: intel/llvm/pull/12200

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9f88cf8) 15.73% compared to head (64b99d2) 15.73%.
Report is 7 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1194   +/-   ##
=======================================
  Coverage   15.73%   15.73%           
=======================================
  Files         223      223           
  Lines       31477    31478    +1     
  Branches     3558     3558           
=======================================
+ Hits         4953     4954    +1     
  Misses      26473    26473           
  Partials       51       51           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[hdelan]

An #ifdef for ROCm version might be a good idea, as well as a comment that the hipMemcpyDefault is broken in HIP. (Maybe even link this ROCm/clr#40 in the comment)

[GeorgeWeb]

An #ifdef for ROCm version might be a good idea, as well as a comment that the hipMemcpyDefault is broken in HIP. (Maybe even link this ROCm/clr#40 in the comment)

Updated this now, changes applicable to using ROCm HIP version 5.6.0 and above only.

[mmoadeli]

LGTM.
A minor issue might be using empty curly braces on declrations, which seems inconsistent with the rest of the file.

[hdelan]

Why did you close this @GeorgeWeb ? We could still use this fix

[GeorgeWeb]

Why did you close this @GeorgeWeb ? We could still use this fix

Didn't mean to actually. Re-opening now.

[GeorgeWeb]

Pinging @npmiller @hdelan @mmoadeli for final thoughts. Thanks!

Based on discussion in the original issue raised in ROCm/clr it seems this workaround will be needed as the fix is not going to be cherry-picked into older releases. Also it is unclear which of the next releases will have the fix, when that is clear this patch can be ifdef'd for the range of releases starting from ROCm 5.6.0 until ROCm 6.x that has their fix.

[hdelan]

LGTM. I might just add a comment to say TODO: Add HIP max version when bug has been fixed just at the ifdef

[GeorgeWeb]

TODO: Add HIP max version when bug has been fixed

Yeah makes sense. Easy to lose track of these.

[npmiller]

Pinging for final thoughts. Thanks!

Based on discussion in the original issue raised in ROCm/clr it seems this workaround will be needed as the fix is not going to be cherry-picked into older releases. Also it is unclear which of the next releases will have the fix, when that is clear this patch can be ifdef'd for the range of releases starting from ROCm 5.6.0 until ROCm 6.x that has their fix.

That reasoning makes sense to me