Skip to content

[TOSA] Fix output size calculation for pool ops #4125

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

Merged
merged 1 commit into from
Apr 24, 2025

Conversation

justin-ngo-arm
Copy link
Contributor

TOSA requires (inputDim + padBefore + padAfter - kernel) to be fully divisible by stride. This update adds pad and input size modifications for pooling ops (AvgPool2d and MaxPool2d) to satisfy that requirement by TOSA.

@sjarus sjarus requested a review from sahas3 April 14, 2025 17:49
@justin-ngo-arm
Copy link
Contributor Author

@sahas3, I have updated my code according to your comments as well as added some e2e tests.

Copy link
Member

@sahas3 sahas3 left a comment

Choose a reason for hiding this comment

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

LGTM.

TOSA requires (inputDim + padBefore + padAfter - kernel) to be fully
divisible by stride. This update adds pad and input size modifications
for pooling ops (AvgPool2d and MaxPool2d) to satisfy that requirement
by TOSA.

Signed-off-by: Justin Ngo <[email protected]>
Change-Id: Iab4021f2dda87cb87e54e4e9ca20bd3688dc1c50
@justin-ngo-arm
Copy link
Contributor Author

@vivekkhandelwal1, can you help me retrigger the CI for this PR please? CI failed because of time out due to unavailable resources.

@vivekkhandelwal1
Copy link
Collaborator

@vivekkhandelwal1, can you help me retrigger the CI for this PR please? CI failed because of time out due to unavailable resources.

Done

@justin-ngo-arm
Copy link
Contributor Author

@vivekkhandelwal1 can you help me check the CI please? I'm not sure what's wrong with it.

@vivekkhandelwal1, can you help me retrigger the CI for this PR please? CI failed because of time out due to unavailable resources.

Done

@sjarus sjarus merged commit bff2a99 into llvm:main Apr 24, 2025
3 checks passed
@sahas3
Copy link
Member

sahas3 commented Apr 25, 2025

@justin-ngo-arm, this change caused a LIT failure in Windows CI https://github.com/llvm/torch-mlir/actions/runs/14649870432/job/41112829976#step:8:5957 likely because tosa.const_shape ops are not emitted in deterministic order.

@vivekkhandelwal1
Copy link
Collaborator

After this PR was merged, the Windows build started failing https://github.com/llvm/torch-mlir/actions/runs/14649870432/job/41112829976. @justin-ngo-arm can you please take a look?

@justin-ngo-arm
Copy link
Contributor Author

@sahas3, @vivekkhandelwal1, I'm working on a PR to fix this. I will submit it ASAP.

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.

None yet

4 participants