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

[TorchToLinalg] Add aten.fft_rfft and lowering #3857

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

giacs-epic
Copy link
Contributor

@giacs-epic giacs-epic commented Nov 7, 2024

  • Add AtenFftRfftOp to Torch dialect.
  • Add conversion of AtenFftRfftOp to Linalg, using a linalg.matmul per output component (real and imaginary). Computing the DFT is O(n^2).
  • Add decomposition of AtenFftRfftOp into Torch-level ops (same paradigm as above).
  • Add unit and end-to-end tests.

Copy link
Collaborator

@zjgarvey zjgarvey left a comment

Choose a reason for hiding this comment

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

Before reviewing in detail, let me see if I understand correctly what your cross-repository goal is.

  1. You include this torch-to-linalg conversion as a baseline conversion, but in IREE, you intend to have a more performant lowering to linalg_ext? I suppose this pass exists before torch-to-linalg in the torch-to-iree pipeline?
  2. Would it make more sense to add this as a decomposition of the op at the torch-dialect level? That way, other backends like Tosa and StableHLO could benefit, and we can turn off the op via backend-legal-ops option in torch-decompose-complex-ops pass if we want to go a different route in IREE. I have plans to modify the decompose complex ops pass to be more specific in the torch-to-iree pipeline this week, so we can specify a backend-legal-ops set there.

@giacs-epic giacs-epic changed the title [TorchToLinalg] Add aten.ffr_rfft and lowering [TorchToLinalg] Add aten.fft_rfft and lowering Nov 11, 2024
@giacs-epic
Copy link
Contributor Author

giacs-epic commented Nov 11, 2024

@zjgarvey

  1. Yes, that's the goal so far. Indeed I would place a pass to lower rfft to linalg_ext before torch-to-linalg in iree.
  2. That makes a lot of sense. Would it be compatible with an eventual decomposition of aten.stft? I.e. would having both decompositions yield to the following behavior: aten.stft gets decomposed into aten.fft_rffts, which in turn get decomposed into matmuls?

@zjgarvey
Copy link
Collaborator

2. That makes a lot of sense. Would it be compatible with an eventual decomposition of `aten.stft`? I.e. would having both decompositions yield to the following behavior: `aten.stft` gets decomposed into `aten.fft_rfft`s, which in turn get decomposed into matmuls?

Yeah, precisely. There are some limitations, however. Does the higher performance path for fft_rfft to linalg_ext apply to the same cases as this conversion? If not, we will definitely need to keep this as a torch-to-linalg conversion to catch any patterns that failed to match the conversion to linalg_ext. This is because we won't be able to go back to decompose-complex-ops after trying to convert to linalg_ext.

@zjgarvey
Copy link
Collaborator

Ah, I see you already converted this to a decomposition. Perhaps we should just do both? StableHlo and Tosa would benefit from the decomposition, which we can turn off once you add the torch-to-linalg-ext path to IREE, and then the torch-to-linalg conversion would be a final fallback if the linalg_ext path doesn't apply.

@giacs-epic
Copy link
Contributor Author

giacs-epic commented Nov 13, 2024

@zjgarvey The higher-performance path would apply when the input signal length is a power of 2, all other cases would need to be translated to this "naive" algorithm. Do you think it's possible to branch compilation based on the input dimension size?
Otherwise I'm open to just keeping both decomposition and lowering to linalg.

@zjgarvey
Copy link
Collaborator

It might be possible to mark the op as conditionally illegal for decompose-complex-ops, but I don't think we want to go that route. Let's add both the torch-to-linalg conversion and the decomposition for now.

@giacs-epic
Copy link
Contributor Author

@zjgarvey Added conversion back.

@zjgarvey zjgarvey self-requested a review November 14, 2024 18:03
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