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] Adds basics to implement HIP HAL driver #15506

Merged
merged 4 commits into from
Nov 30, 2023

Conversation

nithinsubbiah
Copy link
Collaborator

This commit starts a HIP backend in HAL. Following the steps of the new CUDA backend (cuda2) (#13245) effort, HIP will provide enhancements over existing ROCm HAL backend which is based on the current CUDA backend. This commit can be built with -DIREE_EXTERNAL_HAL_DRIVERS=hip which will initialize the HIP driver. Device registration can be verified using tools/iree-run-module --dump_devices.

Copy link
Contributor

@nirvedhmeshram nirvedhmeshram left a comment

Choose a reason for hiding this comment

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

I am suspecting you are copying cuda rather than experimental/cuda2 (since I see status_util files which are not there in cuda2)? I think we want to follow cuda2 which is a more modern implementation.

experimental/hip/hip_driver.c Outdated Show resolved Hide resolved
experimental/hip/hip_driver.c Outdated Show resolved Hide resolved
@antiagainst
Copy link
Contributor

Thanks for the patch! I didn't get to it today; will take a look tomorrow.

experimental/hip/CMakeLists.txt Outdated Show resolved Hide resolved
experimental/hip/CMakeLists.txt Outdated Show resolved Hide resolved
experimental/hip/dynamic_symbol_tables.h Show resolved Hide resolved
experimental/hip/dynamic_symbol_tables.h Outdated Show resolved Hide resolved
experimental/hip/dynamic_symbol_tables.h Outdated Show resolved Hide resolved
experimental/hip/hip_driver.c Outdated Show resolved Hide resolved
experimental/hip/hip_driver.c Outdated Show resolved Hide resolved
experimental/hip/hip_driver.c Outdated Show resolved Hide resolved
experimental/hip/hip_driver.c Outdated Show resolved Hide resolved
experimental/hip/hip_driver.c Outdated Show resolved Hide resolved
@antiagainst
Copy link
Contributor

BTW, when addressing comments, please add additional commits (no force push) so that it's easier to identify what's changed. If you want to sync up with main, please do git merge origin/main so that it's not rebasing the history and we can easily tell what's changed due to merging. Thanks! :)

This commit starts a HIP backend in HAL. Following the steps of CUDA rewrite (iree-org#13245) effort, this backend will provide improvements over the existing ROCm HAL backend. Building this commmit with -DIREE_EXTERNAL_HAL_DRIVERS=hip will initialize this driver and the driver information can be seen using `tools/iree-run-module --dump_devices`.
Addresses comments from the earlier patch
@nithinsubbiah
Copy link
Collaborator Author

BTW, when addressing comments, please add additional commits (no force push) so that it's easier to identify what's changed. If you want to sync up with main, please do git merge origin/main so that it's not rebasing the history and we can easily tell what's changed due to merging. Thanks! :)

Sorry, I didn't know the convention and I had already rebased and started addressing comments before I saw this. Will follow the right steps from next patch.

Copy link
Contributor

@antiagainst antiagainst left a comment

Choose a reason for hiding this comment

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

Looks nice. A couple of small issues still.

experimental/hip/CMakeLists.txt Outdated Show resolved Hide resolved
experimental/hip/dynamic_symbol_tables.h Outdated Show resolved Hide resolved
experimental/hip/dynamic_symbol_tables.h Outdated Show resolved Hide resolved
experimental/hip/dynamic_symbols.c Outdated Show resolved Hide resolved
experimental/hip/dynamic_symbols.h Outdated Show resolved Hide resolved
experimental/hip/status_util.h Show resolved Hide resolved
#include "experimental/hip/dynamic_symbols.h"
#include "iree/base/status.h"

// TODO: Map HIP error strings with their corresponding IREE error state
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you planning to do this next?

experimental/hip/hip_driver.c Outdated Show resolved Hide resolved
Copy link
Contributor

@antiagainst antiagainst left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the comments; LGTM now! But don't submit yet; we'd like @benvanik to take a look too.

@stellaraccident
Copy link
Collaborator

@benvanik ping for review

@nithinsubbiah
Copy link
Collaborator Author

@antiagainst could you please merge since I don't have write access?

@Groverkss Groverkss merged commit ef51eb6 into iree-org:main Nov 30, 2023
57 checks passed
ramiro050 pushed a commit to ramiro050/iree that referenced this pull request Dec 19, 2023
This commit starts a HIP backend in HAL. Following the steps of the new
CUDA backend (`cuda2`) (iree-org#13245)
effort, HIP will provide enhancements over existing ROCm HAL backend
which is based on the current CUDA backend. This commit can be built
with `-DIREE_EXTERNAL_HAL_DRIVERS=hip` which will initialize the HIP
driver. Device registration can be verified using `tools/iree-run-module
--dump_devices`.
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.

None yet

6 participants