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

[uncategorized_lowerings] Add lowering for torch.aten.round.decimals #3811

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

meshtag
Copy link
Member

@meshtag meshtag commented Oct 22, 2024

Implement missing lowering for the op in a similar fashion as done by torch inductor. Also fix data movement and reduce op variants patterns to correctly handle explicitly declared legal ops.

Inductor decomposition ref: https://github.com/pytorch/pytorch/blob/main/torch/_inductor/decomposition.py#L223.

@meshtag meshtag force-pushed the prathamesh/aten.round.decimals branch from c7a3666 to 286443f Compare October 22, 2024 06:02
Implement missing lowering for the op in a similar fashion as done by torch
inductor. Also fix data movement and reduce op variants patterns to correctly
handle explicitly declared legal ops.

Signed-off-by: Prathamesh Tagore <[email protected]>
Copy link
Collaborator

@vivekkhandelwal1 vivekkhandelwal1 left a comment

Choose a reason for hiding this comment

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

I am not able to understand why the lowering for the op torch.aten.round.decimals is added as a custom op lowering, why don't do it the right way?

Please follow the steps specified here https://github.com/llvm/torch-mlir/blob/main/docs/Torch-ops-E2E-implementation.md to add the op lowering.

@vivekkhandelwal1
Copy link
Collaborator

I am not able to understand why the lowering for the op torch.aten.round.decimals is added as a custom op lowering, why don't do it the right way?

Please follow the steps specified here https://github.com/llvm/torch-mlir/blob/main/docs/Torch-ops-E2E-implementation.md to add the op lowering.

Hi @meshtag, is this PR still of interest to you?

@meshtag
Copy link
Member Author

meshtag commented Nov 12, 2024

Hi @meshtag, is this PR still of interest to you?

Hey @vivekkhandelwal1, looks like I missed this PR. Apologies for that.

I am not able to understand why the lowering for the op torch.aten.round.decimals is added as a custom op lowering, why don't do it the right way?

AFAICT, we have two options here:

  1. Get this op directly in the mlir-world and then lower it from there.
  2. Register this op for decomposition (it already exists in torch, as pointed above) and then decompose it at a higher level (ideally before the fx->mlir translation layer) and then directly handle the decomposed op (which is already supported).

This PR tries to go via path 1. I am not sure if we should discard path 2 though, can you please share your thoughts on this.

Please follow the steps specified here https://github.com/llvm/torch-mlir/blob/main/docs/Torch-ops-E2E-implementation.md to add the op lowering.

Sure, I can do this (if we want to go via this path). Thanks for pointing it out.

@vivekkhandelwal1
Copy link
Collaborator

vivekkhandelwal1 commented Nov 14, 2024

I am not able to understand why the lowering for the op torch.aten.round.decimals is added as a custom op lowering, why don't do it the right way?

AFAICT, we have two options here:

  1. Get this op directly in the mlir-world and then lower it from there.
  2. Register this op for decomposition (it already exists in torch, as pointed above) and then decompose it at a higher level (ideally before the fx->mlir translation layer) and then directly handle the decomposed op (which is already supported).

This PR tries to go via path 1. I am not sure if we should discard path 2 though, can you please share your thoughts on this.

If you want to go thorough the path 1, then I don't think you have to do much. Just register this op in Torch-MLIR, and the same lowering which you have written can be used. Also, you would be able to test the correctness of your lowering e2e.

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.

2 participants