-
Notifications
You must be signed in to change notification settings - Fork 610
[MLIR][TORCH] Add E2E support for max_pool2d_with_indices op #518
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not done reviewing. In the meantime, here are a few comments:
23674a7 to
971f70e
Compare
|
I have not rebased it over the latest upstream changes because the newest commit creates a linker error. |
e2e_testing/torchscript/basic.py
Outdated
| @export | ||
| @annotate_args([ | ||
| None, | ||
| ([-1, -1, -1, -1], torch.float32, True), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be nice to also have a test for the 3d input case, to make sure it is also being handled correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the current implementation of maxpool2d supports 3-d cases at present.
971f70e to
51d6140
Compare
|
Please don't re-review the code as of now. I have just resolved the conflicts, working on fixing the issues. |
51d6140 to
22d1ef6
Compare
|
@ramiro050 @cathyzhyi I have updated the code. Please review this when you find the time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More consideration needs to be given to cases maximum value appear multiple times in a window. I put some comments related to this. We need a test for such case as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few drive-by comments
22d1ef6 to
62fe70a
Compare
|
@cathyzhyi @ramiro050 Just a gentle reminder for your review. |
62fe70a to
5ea706a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a testcase when there are duplicating maximum value in a window? An input tensor with all ones would be good enough.
This commit adds lowering of `max_pool2d_with_indices` op. Signed-Off By: Vivek Khandelwal <vivek@nod-labs.com>
5ea706a to
9238d2b
Compare
Co-authored-by: Kevin O'Brien <caomhin@us.ibm.com>
This commit adds lowering of
max_pool2d_with_indicesop.Signed-Off By: Vivek Khandelwal vivek@nod-labs.com