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

[mlir] Use find_package() to locate MLIR static library #1673

Merged
merged 30 commits into from
Sep 30, 2022
Merged

Conversation

zhanglx13
Copy link
Contributor

@zhanglx13 zhanglx13 commented Aug 4, 2022

The latest version of MLIR exports the static library that can be found by find_package(). The changes made on the MLIR side can be found here.

New names (static library related):

  • MLIR project name: rocMLIR
  • MLIR package name: rocMLIR
  • MLIR static library name: rockCompiler

The corresponding changes on the MIOpen side are:

  • Use find_package(rocMLIR 1.0.0) to find the rocMLIR package of version 1.0.0. This is the first version of MLIR that exports the static library.
  • For backward compatibility, use find_library() to locate the library in the old way if find_package() does not find the package.
  • Add rocMLIR::rockCompiler to the link library of target MIOpen.
  • Update the commit id of MLIR in requirements.txt.

@jerryyin
Copy link
Member

jerryyin commented Aug 4, 2022

@zhanglx13 Could you update https://github.com/ROCmSoftwarePlatform/MIOpen/blob/develop/requirements.txt so that on MIOpen end it can pull in latest commit and run through MIOpen regressions?

@jerryyin
Copy link
Member

jerryyin commented Aug 4, 2022

@zhanglx13 FYI this is a ROCm 5.4 feature so can only merge after MIOpen branches.

Could you take a look at test failures and try to work through? Just so that when MIOpen finished branching we are good to go.

@krzysz00
Copy link
Contributor

krzysz00 commented Aug 4, 2022

I thought MIOpen had branched already? In any case, this is still blocking deploying my changes because of the order the changes are going into MLIR.

driver/CMakeLists.txt Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
@zhanglx13 zhanglx13 marked this pull request as ready for review August 15, 2022 14:07
@zhanglx13 zhanglx13 changed the title Use find_package() to locate MLIR static library [mlir] Use find_package() to locate MLIR static library Aug 17, 2022
@zhanglx13
Copy link
Contributor Author

zhanglx13 commented Aug 19, 2022

@junliume Do you know how to rebuild the docker image? For now, Jenkins let me set the parameter DOCKER_IMAGE_OVERRIDE but it does not rebuild the image if a new name is given.

The last successful CI build shows that the compatibility function is working well

[2022-08-18T21:39:49.140Z] -- Could NOT find Miir (missing: Miir_DIR)
[2022-08-18T21:39:49.140Z] -- Falling back to find library libMLIRMIOpen
[2022-08-18T21:39:49.140Z] -- Build with library libMLIRMIOpen: /opt/rocm/lib/libMLIRMIOpen.a

And this also shows that the image used contains the old version of MLIR static library. To test the new exported library, a image needs to be built with the updated version of MLIR in the requirements.txt.

CMakeLists.txt Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
@JehandadKhan
Copy link
Contributor

@junliume Do you know how to rebuild the docker image? For now, Jenkins let me set the parameter DOCKER_IMAGE_OVERRIDE but it does not rebuild the image if a new name is given.

The last successful CI build shows that the compatibility function is working well

[2022-08-18T21:39:49.140Z] -- Could NOT find Miir (missing: Miir_DIR)
[2022-08-18T21:39:49.140Z] -- Falling back to find library libMLIRMIOpen
[2022-08-18T21:39:49.140Z] -- Build with library libMLIRMIOpen: /opt/rocm/lib/libMLIRMIOpen.a

And this also shows that the image used contains the old version of MLIR static library. To test the new exported library, a image needs to be built with the updated version of MLIR in the requirements.txt.

Hello @zhanglx13. You can rebuild the docker yourself with the docker build arguments that our CI echos, push it to either the internal or the external registry and then override the CI runs using the DOCKER_IMAGE_OVERRIDE parameter. Once your PR is merged we can update the image that the CI uses by default.

@zhanglx13
Copy link
Contributor Author

The old MLIR is checked with build 32.
The new MLIR is checked with build 40.

jerryyin
jerryyin previously approved these changes Aug 22, 2022
@jerryyin jerryyin added this to the ROCm 5.4 milestone Aug 25, 2022
shurale-nkn
shurale-nkn previously approved these changes Aug 26, 2022
Copy link
Contributor

@shurale-nkn shurale-nkn left a comment

Choose a reason for hiding this comment

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

LGTM

@zhanglx13
Copy link
Contributor Author

@junliume I haven't seen a successful build of the docker image with the new dependencies. Still trying to figure out the cause.

@zhanglx13
Copy link
Contributor Author

There is a blocker on the CK side.
After ROCm/composable_kernel#435 lands, we need to update the CK commit id in requirements and rebuild the docker image.

@zhanglx13
Copy link
Contributor Author

Another issue caused by CK integration. See #1777 for details.

@zhanglx13
Copy link
Contributor Author

Decision needs to be made by @junliume @JehandadKhan about CK integration. See #1777 for details.
If it is decided to bound with an old version of CK, then we need to revert fd08663 and update CK's commit id in requirements.txt to the chosen one.
If it is decided to keep up with the new CK, we can wait until all issues are resolved.

CMakeLists.txt Outdated Show resolved Hide resolved
@zhanglx13
Copy link
Contributor Author

Blocked by PR #1761.

@krzysz00
Copy link
Contributor

I'd just like to reiterate that the changes in rocMLIR that require this PR to land are already in our 5.4 release branch, and so therefore this PR must land on MIOpen's 5.4 release branch - or at least that's what I can tell of the situation

requirements.txt Outdated Show resolved Hide resolved
@junliume junliume merged commit 5c66640 into develop Sep 30, 2022
@zhanglx13
Copy link
Contributor Author

zhanglx13 commented Sep 30, 2022

@junliume build#80 has all greens. However, rocm/miopen:ci does not use the rocm-5.4 version of MLIR. Therefore, I kicked off build#81, which builds a new docker image with the latest MLIR. I think we should be safe to merge this PR after we see all greens in build#81.

@junliume junliume deleted the find-MLIR branch October 10, 2022 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants