Enable dpnp build on AMD GPU #2302
Conversation
vlad-perevezentsev
requested review from
antonwolfy,
AlexanderKalistratov and
vtavana
as code owners
|
Array API standard conformance tests for dpnp=0.18.0dev0=py312he4f9c94_45 ran successfully. |
|
View rendered docs @ https://intelpython.github.io/dpnp/pull/2302/index.html |
scripts/build_locally.py
Outdated
| if not arch: | ||
| raise ValueError("--arch is required when --target=hip") | ||
| cmake_args += [ | ||
| "-DDPNP_TARGET_HIP=ON", |
There was a problem hiding this comment.
For what do we need to define two variables? Can it be combined in a single one, like in dpctl: -DDPNP_TARGET_HIP={arch}?
There was a problem hiding this comment.
Additionally, --target=cuda is current dpnp approach, but:
- dpctl and dpnp should consider supporting targeting specific CUDA architectures
--target=hipmeans that there is no way to build simultaneously for HIP and CUDA (which is very, very much an edge case, but should be considered)
For these reasons, I think it is most sensible to move away from --target= universal approach to --target-cuda= and --target-hip= or something to that effect
There was a problem hiding this comment.
@ndgrigorian it is a great suggestion.
I have added support for --target-hip and I am going to add --target-cuda instead of --target in the next PR.
Thanks
|
|
||
| .. code-block:: bash | ||
|
|
||
| python scripts/build_locally.py --target-hip=gfx90a |
There was a problem hiding this comment.
In general it might be unclear what gfx90a means here. It'd be great to clarify.
| @@ -104,6 +105,15 @@ def run( | |||
| # Always builds using oneMKL interfaces for the cuda target | |||
| onemkl_interfaces = True | |||
|
|
|||
| if target_hip is not None: | |||
| if target_hip == "default": | |||
There was a problem hiding this comment.
We need a special handling for python scripts/build_locally.py --target-hip=.
Now it is equal to python scripts/build_locally.py, which was not intended, I guess.
There was a problem hiding this comment.
The same comment is applicable to python scripts/build_locally.py --target=.
| @@ -104,6 +105,15 @@ def run( | |||
| # Always builds using oneMKL interfaces for the cuda target | |||
| onemkl_interfaces = True | |||
|
|
|||
| if target_hip is not None: | |||
| if target_hip == "default": | |||
There was a problem hiding this comment.
It is a bit unclear what the use case assumed here? Is it about python scripts/build_locally.py --target-hip="default" only?
Then I believe the error message below needs to be rephrase a bit to something like No default HIP architecture is supported. It must be specified explicitly.
| @@ -75,27 +75,64 @@ option(DPNP_USE_ONEMKL_INTERFACES | |||
| "Build DPNP with oneMKL Interfaces" | |||
| OFF | |||
| ) | |||
| set(HIP_TARGETS "" CACHE STRING "HIP architecture for target") | |||
There was a problem hiding this comment.
I assume there is no support for multiple values:
| set(HIP_TARGETS "" CACHE STRING "HIP architecture for target") | |
| set(HIP_TARGET "" CACHE STRING "HIP architecture for target") |
| set(_dpnp_sycl_targets ${DPNP_SYCL_TARGETS}) | ||
| set(_dpnp_sycl_targets ${DPNP_SYCL_TARGETS}) | ||
|
|
||
| if (NOT "x${HIP_TARGETS}" STREQUAL "x") |
There was a problem hiding this comment.
Why is that applicable only to HIP target? What is the use case? Should it be supported for CUDA target also?
|
|
||
| if (NOT "x${HIP_TARGETS}" STREQUAL "x") | ||
| set(_dpnp_amd_targets ${HIP_TARGETS}) | ||
| set(_use_onemkl_interfaces_hip ON) |
There was a problem hiding this comment.
Do we need here something similar to above?
set(_dpnp_sycl_targets "amdgcn-amd-amdhsa,${_dpnp_sycl_targets}")
This PR updates
СMakeListsfiles andbuild_locally.pyto enable building dpnp for AMD targets.To build dpnp on AMD:
To find the architecture, use