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

[MIGraphX EP] enable compilation and execution on Windows #36

Merged

Conversation

apwojcik
Copy link

The PR changes CMake files and ONNXRUNTIME source code to enable the compilation of MIGraphX EP on Windows—key differences from the original implementation.

  • separated ROCm EP from MIGraphX EP
  • MIGraphX EP no longer depends on rocBLAS and MIOpen
  • replaced ROCm allocators with MIGRaphX allocators
    Microsoft Visual C++ 2022 compiler has been used to compile ONNXRT, including EP on Windows.

@apwojcik apwojcik added enhancement New feature or request windows labels May 17, 2024
Copy link
Collaborator

@jeffdaily jeffdaily left a comment

Choose a reason for hiding this comment

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

Minor changes requested. If you want to upstream this PR, be aware of the linting.

cmake/onnxruntime_providers_migraphx.cmake Show resolved Hide resolved
cmake/onnxruntime_providers_migraphx.cmake Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@apwojcik apwojcik requested a review from jeffdaily June 3, 2024 11:27
@causten
Copy link

causten commented Jun 10, 2024

I have no comment on the code, but @apwojcik the rocm6.xxx branches do not upstream. They are snap shots of upstream, so you may also need to post to onnxruntime for this change to stick around

@TedThemistokleous
Copy link

@apwojcik this looks good. Have you been able to get any inference on windows running with this?

I've just quickly fixed a conflict from another changes that was added into the rocm6.2_internal_testing branch.

@TedThemistokleous
Copy link

Trying this now to compile and ensure nothing has changed for the linux ep side build. Only real change I see is you're using

Policy "CMP0144" , as we're using cmake 3.26.4, and this requires atleast 3.27.x

We'll have to update some more scripts on our end if that's the case.

Copy link

@TedThemistokleous TedThemistokleous left a comment

Choose a reason for hiding this comment

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

Was able to build and run an EP inference in Linux with one of the MIGraphX examples. This looks good.

Ive added the small change since its one line. Will merge once CI checks out. Likely thats what caused your build failure

@TedThemistokleous TedThemistokleous merged commit 69e234a into rocm6.2_internal_testing Jun 17, 2024
12 of 14 checks passed
TedThemistokleous added a commit that referenced this pull request Jun 17, 2024
* enable MIGraphX EP on Windows

* [MIGraphX EP] Fix provider options

* fix formatting

* unify the package name for both rocm and migraphx

* fix compilation after moving to rocm6.2

* make STREAM_SYNC the default

* workaround hip sdk bug on windows

* Revert rename of private var for now

---------

Co-authored-by: Filip Jankovic <[email protected]>
Co-authored-by: Ted Themistokleous <[email protected]>
Co-authored-by: Ted Themistokleous <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants