Switch to upstream StablehloToLinalg code. #19792
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ScottTodd
added
cleanup 🧹
integrations/stablehlo
|
-12k O_O nice! |
ScottTodd
commented
Jan 23, 2025
compiler/plugins/input/StableHLO/Conversion/StableHLOToIREEInputDialects.cpp
Outdated
Show resolved
Hide resolved
ScottTodd
commented
Jan 23, 2025
abhigunj
pushed a commit
to openxla/stablehlo
that referenced
this pull request
Jan 23, 2025
This function declaration has existed since the code was forked from IREE in #1817, but the implementation was kept private (static function within an anonymous namespace). I'm now trying to switch IREE from having its own implementation to using the upstream implementation from this project in iree-org/iree#19792, and I would like to access these patterns directly, instead of through the `StablehloLegalizeToLinalgPass`. With the patterns I can run conversion including my own sets of additional patterns, while a pass runs in isolation. I'm also deleting the `populateLegalizeChloPatterns`, `populateLegalizeControlFlowPatterns`, and `populateLegalizeShapeComputationPatterns` declarations, which were not migrated from IREE and are also dangling without implementations.
ScottTodd
changed the title
ScottTodd
requested review from
hanhanW,
MaheshRavishankar and
rsuderman
as code owners
|
Got all in-tree presubmit tests passing. Ready for review. |
ScottTodd
commented
Jan 27, 2025
compiler/plugins/input/StableHLO/Conversion/StableHLOToIREEInputDialects.cpp
Outdated
Show resolved
Hide resolved
kuhar
approved these changes
Jan 27, 2025
ita9naiwa
pushed a commit
to ita9naiwa/iree
that referenced
this pull request
Feb 4, 2025
While looking at compiler warnings in build logs, I noticed paths in StableHLO that looked out of place. As it turns out, much of IREE's StableHLO to Linalg conversion code was forked into upstream StableHLO in openxla/stablehlo#1817, though there have been some local changes to the code here since it was forked: https://github.com/iree-org/iree/commits/main/compiler/plugins/input/StableHLO/Conversion. Switching to use the upstream code will allow us to decrease the surface area we directly support and limit the number of files we need to build from source, but it will also make maintenance require coordinating more with upstream (such as during LLVM integrates). We still point to a fork at https://github.com/iree-org/stablehlo , so if things get tricky we can choose to set up a branch with patches as needed. Some notes: * More code, particularly includes and build dependencies, could be pruned. * We can probably delete more code by reviving iree-org#18681 too * I deleted lit tests for the patterns that were moved upstream. The tests still exist at https://github.com/openxla/stablehlo/tree/main/stablehlo/conversions/linalg/tests, but I don't see much value in having our own versions of the lit tests. We do still have e2e tests that compile and run. * I did _not_ plumb through the `enablePrimitiveOps` or `enableSparseOps` options, which may be useful for some programs * I'm keeping our custom `stablehlo.concatenate` lowering since the alternate lowering (from IREE or now StableHLO) has correctness issues. Also keeping the FFT lowering since that does not exist upstream and it handles cases that our LinalgExt lowering does not. Signed-off-by: Hyunsung Lee <ita9naiwa@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While looking at compiler warnings in build logs, I noticed paths in StableHLO that looked out of place. As it turns out, much of IREE's StableHLO to Linalg conversion code was forked into upstream StableHLO in openxla/stablehlo#1817, though there have been some local changes to the code here since it was forked: https://github.com/iree-org/iree/commits/main/compiler/plugins/input/StableHLO/Conversion.
Switching to use the upstream code will allow us to decrease the surface area we directly support and limit the number of files we need to build from source, but it will also make maintenance require coordinating more with upstream (such as during LLVM integrates). We still point to a fork at https://github.com/iree-org/stablehlo , so if things get tricky we can choose to set up a branch with patches as needed.
Some notes:
enablePrimitiveOpsorenableSparseOpsoptions, which may be useful for some programsstablehlo.concatenatelowering since the alternate lowering (from IREE or now StableHLO) has correctness issues. Also keeping the FFT lowering since that does not exist upstream and it handles cases that our LinalgExt lowering does not.