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

[Stablehlo] add refbackend #2712

Merged
merged 4 commits into from
Feb 15, 2024
Merged

Conversation

qingyunqu
Copy link
Collaborator

No description provided.

@qingyunqu qingyunqu marked this pull request as draft January 15, 2024 15:40
@qingyunqu qingyunqu changed the title WIP: [Stablehlo] add refbackend [Stablehlo] add refbackend Jan 15, 2024
@qingyunqu qingyunqu marked this pull request as ready for review February 15, 2024 15:39
Copy link
Collaborator

@stellaraccident stellaraccident left a comment

Choose a reason for hiding this comment

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

I'm fine with giving this a try, but the stablehlo ref backend significantly increases the surface area of the stablehlo dep, and keeping it up to date has already been a challenge.

I think this pushes us to a policy change: if when integrating a new version of llvm, if a corresponding stablehlo version doesn't work, we will land with stablehlo disabled and fix it in post commit. If this becomes a pattern, we can factor the CI a bit differently to test these other configurations in a non gating way.

I think that removing the requirement that every commit is available in lockstep will give us all what we want without needing to engage in integrate heroics.

@qingyunqu
Copy link
Collaborator Author

I'm fine with giving this a try, but the stablehlo ref backend significantly increases the surface area of the stablehlo dep, and keeping it up to date has already been a challenge.

I think this pushes us to a policy change: if when integrating a new version of llvm, if a corresponding stablehlo version doesn't work, we will land with stablehlo disabled and fix it in post commit. If this becomes a pattern, we can factor the CI a bit differently to test these other configurations in a non gating way.

I think that removing the requirement that every commit is available in lockstep will give us all what we want without needing to engage in integrate heroics.

I agree with you. I think it's ok to disable stablehlo temporarily.
BTW, this PR is ready. Could you please review it?

Copy link
Collaborator

@stellaraccident stellaraccident left a comment

Choose a reason for hiding this comment

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

Thanks! Before landing, would you mind conditionalizing like I mention in the comments?

lib/CMakeLists.txt Outdated Show resolved Hide resolved
lib/InitAll.cpp Show resolved Hide resolved
lib/InitAll.cpp Show resolved Hide resolved
@qingyunqu
Copy link
Collaborator Author

Thanks! Before landing, would you mind conditionalizing like I mention in the comments?

Thanks, I forgot these config.

@qingyunqu qingyunqu merged commit f3e8199 into llvm:main Feb 15, 2024
3 checks passed


class LinalgOnTensorsStablehloBackend(StablehloBackend):
"""Main entry-point for the linalg-on-tensors based TOSA backend.
Copy link
Member

Choose a reason for hiding this comment

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

Why is this file mentioning TOSA?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a typo, I will fix it.

Comment on lines +34 to +36
if(TORCH_MLIR_ENABLE_STABLEHLO)
list(APPEND LinkedLibs StablehloPasses StablehloLinalgTransforms)
endif()
Copy link
Member

Choose a reason for hiding this comment

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

I'm having trouble compiling some part of this after syncing past this commit (f3e8199). My build is fine before this commit (8e2e5ee) and after this commit with -DTORCH_MLIR_ENABLE_STABLEHLO=OFF.

Error logs: https://gist.github.com/ScottTodd/332e4e76b744d0bfdd75c68cc8191970#file-torch_mlir_build-txt-L1784
System: Windows 10, MSVC (Visual Studio Build Tools 2022 Preview - amd64)

Do StableHLO and torch-mlir have Windows CI builds? I don't see issues in IREE's downstream Windows CI or locally, so either that is behind this version or something else is different about how the projects are configured.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Torch-MLIR doesn't have Windows CI builds now. I don't have an idea to fix this compiling error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems the same error as openxla/stablehlo#1772

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

3 participants