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

Pass some decompose-complex-ops options in torch-to-iree #19076

Merged
merged 6 commits into from
Nov 13, 2024

Conversation

zjgarvey
Copy link
Contributor

@zjgarvey zjgarvey commented Nov 8, 2024

Some problematic decompositions in torch-decompose-complex-ops are now disabled by default in the torch-to-iree pipeline. Also allows turning off all decompositions with an option.

@zjgarvey
Copy link
Contributor Author

zjgarvey commented Nov 8, 2024

I've tried setting the default backendLegalOps in TorchToIREELoweringPipelineOptions, but I think there is something that goes wrong with checking against the default being modified when the option is a list. Maybe since the list doesn't have external storage, even if the contents are the "same" perhaps it is still is technically changed? I might have some confusion about where ireeCompilerSessionSetFlags gets the default values. I assume it gets them from the OptionsBinder, but I'm not sure how that relates to the default value in the TorchToIREELoweringPipelineOptions?

Anyway, this is failing iree/compiler/bindings/python/test/api/api_test because non-default args gotten from the compiler session is returning the backendLegalOps set, and I'm not really sure what the best way around this is without just hacking the option through as not an actual option. Honestly, anything is better than just decomposing all torch-ops without inspection.

@zjgarvey
Copy link
Contributor Author

zjgarvey commented Nov 9, 2024

Okay, removing the backendLegalOps from the OptionsBinder seems to make that test pass.

The downside is that we can't configure the backendLegalOps set from the command line, but frankly I'm okay with that. The only route we are taking with this option is to convert torch to linalg, so the list of backendLegalOps should be standardized anyway.

compiler/plugins/input/Torch/PluginRegistration.cpp Outdated Show resolved Hide resolved
Comment on lines 24 to 28
ListOption<std::string> backendLegalOps{
*this, "backend-legal-ops",
llvm::cl::desc("List of ops to be considered legal by "
"torch-decompose-complex-ops. Use this for ops that have "
"disadvantageous decompositions for our 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 an option at this level? Shouldn't this be set locally where it is used? I can't quite tell from the PR context if this is a user-facing CLI option, but I wouldn't want any user to need to pass a list of "legal" ops to the compiler - the compiler should be able to make that determination on its own.

Also remember: IREE can multi-target, so any decisions made at the input pipeline level must be "backend"-agnostic, where "backend" here means https://github.com/iree-org/iree/tree/main/compiler/plugins/target (cuda, llvm-cpu, metal-spirv, rocm, vmvx, vulkan-spirv, etc.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point.

My understanding is that this is only a user-facing CLI option for iree-opt --torch-to-iree, and not for iree-compile. Do you think I shouldn't pass the backend-legal-ops list as an option at all? I'm totally fine with that.

Copy link
Member

Choose a reason for hiding this comment

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

My understanding is that this is only a user-facing CLI option for iree-opt --torch-to-iree, and not for iree-compile.

I should be able to tell from reading the code... but do the new options appear in iree-compile --help?

Do you think I shouldn't pass the backend-legal-ops list as an option at all? I'm totally fine with that.

A developer option for debugging could be fine, if you or others working on the Torch / ONNX lowerings would use it. I wouldn't do the plumbing for an option unless you have a specific use in mind though. Each branch and option in code takes effort to support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It took me a while to figure this out from the API. The option torch-use-decompose-complex-ops does appear for iree-compile, since is it bound to the OptionBinder, but the backendLegalOps is not configurable from iree-compile, since I am not binding that option.

The signature of create<passname> is a bit restrictive, so I am really only passing this option to be able to pass the list of backendLegalOps from PluginRegistration.cpp into the torch-to-iree pipeline.

Let me think of some ideas on how to best clean this up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ScottTodd Can you take a look at the changes to see if it makes more sense?

I need the backendLegalOps list in two places, and I remember their being some llvm convention on not using global static variables https://llvm.org/docs/CodingStandards.html#do-not-use-static-constructors. So I opted to have a getter function that just returns the list. It will require reconstructing it each time on a function call, but this was the best I could think of to avoid duplicating the hard-coded list (don't want to have to edit it in two places if we need to add another op).

Copy link
Member

Choose a reason for hiding this comment

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

Another way to structure that might be to first run a common decompose ops pass / pipeline then add the mlir::torch::Torch::createTorchOnnxToTorchBackendPipeline and/or TorchInput::createTorchToIREEPipeline. That way there would be only one location that needs the list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I frankly don't know what the purpose of decompose complex ops in the torch-to-iree pipeline is, but I'd like to keep the torch-to-iree pipeline stable to avoid possible regressions unrelated to ONNX for now.

@zjgarvey zjgarvey requested a review from ScottTodd November 12, 2024 19:03
…upstream usage of that option in torch-mlir

Signed-off-by: zjgarvey <[email protected]>
Copy link
Member

@ScottTodd ScottTodd left a comment

Choose a reason for hiding this comment

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

Okay as-is for now. Could explore reordering the pipelines to reduce duplicate code.

@zjgarvey zjgarvey merged commit 68c35d7 into iree-org:main Nov 13, 2024
33 of 36 checks passed
Groverkss pushed a commit to Groverkss/iree that referenced this pull request Dec 1, 2024
…g#19076)

Some problematic decompositions in `torch-decompose-complex-ops` are now
disabled by default in the `torch-to-iree` pipeline. Also allows turning
off all decompositions with an option.

---------

Signed-off-by: zjgarvey <[email protected]>
giacs-epic pushed a commit to giacs-epic/iree that referenced this pull request Dec 4, 2024
…g#19076)

Some problematic decompositions in `torch-decompose-complex-ops` are now
disabled by default in the `torch-to-iree` pipeline. Also allows turning
off all decompositions with an option.

---------

Signed-off-by: zjgarvey <[email protected]>
Signed-off-by: Giacomo Serafini <[email protected]>
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.

3 participants