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

[MHLO] refactor pass configurations #1315

Merged
merged 1 commit into from
Sep 1, 2022
Merged

[MHLO] refactor pass configurations #1315

merged 1 commit into from
Sep 1, 2022

Conversation

tanyokwok
Copy link
Collaborator

Related to #1227

  1. Reduce MHLO #ifdefs
  2. Dismiss compilation warnings

Related to #1227

1. Reduce MHLO #ifdefs
2. Dismiss compilation warnings
Copy link
Collaborator

@Vremold Vremold left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Should we also configure enableI32Index and enableStaticShape in torch-backend-to-mhlo-backend-pipeline, which contains convert-torch-to-mhlo pass?

Copy link
Contributor

@silvasean silvasean left a comment

Choose a reason for hiding this comment

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

Thanks!

@tanyokwok
Copy link
Collaborator Author

Overall LGTM. Could we add this feature to torch-backend-to-mhlo-backend-pipeline registered in lib/Dialect/TorchConversion/Transforms/Passes.cpp, too?

I have no idea if it's the right place to register in lib/Dialect/TorchConversion/Transforms/Passes.cpp. What do you think @silvasean @ZihengJiang

@silvasean
Copy link
Contributor

Overall LGTM. Could we add this feature to torch-backend-to-mhlo-backend-pipeline registered in lib/Dialect/TorchConversion/Transforms/Passes.cpp, too?

I have no idea if it's the right place to register in lib/Dialect/TorchConversion/Transforms/Passes.cpp. What do you think @silvasean @ZihengJiang

We already have this registered there:

#ifdef TORCH_MLIR_ENABLE_MHLO
  mlir::PassPipelineRegistration<Torch::TorchLoweringPipelineOptions>(
      "torch-backend-to-mhlo-backend-pipeline",
      "Pipeline lowering torch backend contract to MHLO backend "
      "contract.",
      TorchConversion::createTorchBackendToMhloBackendPipeline);
#endif

You can replace Torch::TorchLoweringPipelineOptions with a new struct with options parallel to def ConvertTorchToMhlo : Pass<"convert-torch-to-mhlo", "func::FuncOp"> { (this is redundant but required in MLIR now).

Copy link
Collaborator

@ZihengJiang ZihengJiang left a comment

Choose a reason for hiding this comment

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

Look good to me :)

@tanyokwok
Copy link
Collaborator Author

Okk, @Vremold @silvasean I will split it into another PR.

@tanyokwok tanyokwok merged commit 29cafdb into main Sep 1, 2022
@tanyokwok tanyokwok deleted the tanyo/dev branch September 1, 2022 02:36
AmosLewis pushed a commit to AmosLewis/torch-mlir that referenced this pull request Sep 1, 2022
Related to llvm#1227

1. Reduce MHLO #ifdefs
2. Dismiss compilation warnings
AmosLewis pushed a commit to AmosLewis/torch-mlir that referenced this pull request Sep 2, 2022
Related to llvm#1227

1. Reduce MHLO #ifdefs
2. Dismiss compilation warnings
AmosLewis pushed a commit to AmosLewis/torch-mlir that referenced this pull request Sep 2, 2022
Related to llvm#1227

1. Reduce MHLO #ifdefs
2. Dismiss compilation warnings
qedawkins pushed a commit to nod-ai/torch-mlir that referenced this pull request Oct 3, 2022
* Make ONNX operations, and IndexExpr be in the onnx_mlir namespace.

Signed-off-by: Ettore Tiotto <[email protected]>
@silvasean silvasean mentioned this pull request Oct 7, 2022
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.

5 participants