Fall cleaning: Re-organize sources so that each target type is contained and optional. #2442
Conversation
This file contains hidden or 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
stellaraccident
changed the title
stellaraccident
force-pushed
the
isolate_optional_targets
branch
2 times, most recently
from
04dd794 to
14ecd74
Compare
…ned and optional. While working on iree-org/iree#14917, I noticed that it is somewhat hard to take a dependency on torch-mlir such that one only builds deps for the target(s) of interest (in this case Linalg). I noticed that some ifdef'ey optionality was added for stablehlo, but this was not mirrored for the others. Further, it does the switching very deep in the dependency graph vs having top-level directories and defines gating entire features. In addition, I noticed that a lot of things in the Linalg path were broken down to a fine level of detail but were not actually shared/shareable outside of that target. I opted to clump these together into TorchToLinalg. It is easy enough to "promote" them to common with this new organization if the need arises. General approach: * Isolate each conversion target in one of TorchToLinalg, TorchToStablehlo, TorchToTosa. * Gate each by top-level CMake flags and defines. * Common conversions go in a Common/ directory (currently Arith and SCF). * Pull target specific conversions out of TorchConversion/Transforms and put in their top-level directory. * General maintenance on the build graph and registration stuff that had bitrotted and was blocking progress. The main functional change for people taking a source dep is that `#include "torch-mlir/Conversion/Passes.h"` no longer is a one stop shop: For optional conversions, you have to include the dedicated `Passes.h` of each and take a library dep. See `InitAll.cpp` which does it right (and *is* a one stop shop still).
Contributor
|
@stellaraccident Is there a chance that this lands? It nicely decouples the "representation" of Torch in MLIR and the various backends. |
Collaborator
Author
|
I ran into some CI problems and then meant to check with folks if this was desirable -- and then never did. I can finish this off if folks think it is a good idea. |
Contributor
|
I guess this is more a nice to have then a necessity... I think this would be a good idea, but it also seems like there is some work left to do. |
11 tasks
stellaraccident
added a commit
that referenced
this pull request
Nov 22, 2023
Adds a pipeline to convert custom ops and metadata represented as `torch.operator` custom ops to corresponding `torch` ops where possible. This is part of a multi-part approach for building ONNX import in as a regular feature of torch-mlir. It is focused on the conversions vs the infra. We will end up maintaining a [pure-python importer](https://github.com/nod-ai/SHARK-Turbine/blob/main/python/shark_turbine/importers/onnx_importer.py) to go with this in torch-mlir, and we will also maintain test case generation utilities derived from it. I have left substantial documentation in the README of the conversion directory, including the recommended approach that we will take to keep building this out. (note that this organizes the code to coincide with the refactoring in #2442 versus the current flat arrangement)
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 working on iree-org/iree#14917, I noticed that it is somewhat hard to take a dependency on torch-mlir such that one only builds deps for the target(s) of interest (in this case Linalg). I noticed that some ifdef'ey optionality was added for stablehlo, but this was not mirrored for the others. Further, it does the switching very deep in the dependency graph vs having top-level directories and defines gating entire features. In addition, I noticed that a lot of things in the Linalg path were broken down to a fine level of detail but were not actually shared/shareable outside of that target. I opted to clump these together into TorchToLinalg. It is easy enough to "promote" them to common with this new organization if the need arises.
General approach:
The main functional change for people taking a source dep is that
#include "torch-mlir/Conversion/Passes.h"no longer is a one stop shop: For optional conversions, you have to include the dedicatedPasses.hof each and take a library dep. SeeInitAll.cppwhich does it right (and is a one stop shop still).