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] Merge indices and pooling computation into one linalg generic op #3701

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

Conversation

lingzhiz1998
Copy link
Contributor

As discussed in #3652, we should use one linalg generic op to represent maxpool with indices op.

Comment on lines 93 to 99
// CHECK: %[[MAX_PRED:.*]] = arith.cmpf ugt, %[[CURRENT_VALUE]], %[[ACC_OUT]] : f32
// CHECK: %[[MAXF:.*]] = scf.if %[[MAX_PRED]] -> (f32) {
// CHECK: scf.yield %[[CURRENT_VALUE]] : f32
// CHECK: } else {
// CHECK: scf.yield %[[ACC_OUT]] : f32
// CHECK: }
// CHECK-NEXT: linalg.yield %[[MAXF]] : f32
Copy link
Collaborator

Choose a reason for hiding this comment

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

@lingzhiz1998 I haven't gone through the code in this PR, but this IR getting generated instead of the earlier one, is not good for the downstream pipeline, and I don't see a point in doing that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR aims to change the IR of max_pool _with_indices op to eliminate extract op. Generating different IRs for max_pool op is not necessary. The reason of why I did this is making the lowering logic of these ops as identical as possible. If you worry about this, I can keep the IR of max_pool op as same as previous.

@lingzhiz1998 lingzhiz1998 force-pushed the lingzhi/reimplement_maxpoolwithindices branch from 1bd975c to ae1fa20 Compare September 20, 2024 02:48
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