Skip to content

Conversation

@vivekkhandelwal1
Copy link
Collaborator

This commit adds lowering of max_pool2d_with_indices op.

Signed-Off By: Vivek Khandelwal vivek@nod-labs.com

Copy link
Collaborator

@ramiro050 ramiro050 left a 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:

@vivekkhandelwal1 vivekkhandelwal1 force-pushed the aten-maxpool2d-with-indices branch from 23674a7 to 971f70e Compare January 12, 2022 11:04
@vivekkhandelwal1
Copy link
Collaborator Author

I have not rebased it over the latest upstream changes because the newest commit creates a linker error.

@export
@annotate_args([
None,
([-1, -1, -1, -1], torch.float32, True),
Copy link
Collaborator

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

Copy link
Collaborator Author

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.

@vivekkhandelwal1
Copy link
Collaborator Author

Please don't re-review the code as of now. I have just resolved the conflicts, working on fixing the issues.

@vivekkhandelwal1 vivekkhandelwal1 force-pushed the aten-maxpool2d-with-indices branch from 51d6140 to 22d1ef6 Compare April 4, 2022 14:17
@vivekkhandelwal1
Copy link
Collaborator Author

@ramiro050 @cathyzhyi I have updated the code. Please review this when you find the time.

Copy link
Contributor

@cathyzhyi cathyzhyi left a 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.

Copy link
Collaborator

@ramiro050 ramiro050 left a 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

@vivekkhandelwal1
Copy link
Collaborator Author

@cathyzhyi @ramiro050 Just a gentle reminder for your review.

@vivekkhandelwal1 vivekkhandelwal1 force-pushed the aten-maxpool2d-with-indices branch from 62fe70a to 5ea706a Compare April 14, 2022 17:15
Copy link
Contributor

@cathyzhyi cathyzhyi left a 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>
@vivekkhandelwal1 vivekkhandelwal1 force-pushed the aten-maxpool2d-with-indices branch from 5ea706a to 9238d2b Compare April 18, 2022 14:36
@vivekkhandelwal1 vivekkhandelwal1 merged commit 769f3a8 into main Apr 18, 2022
@vivekkhandelwal1 vivekkhandelwal1 deleted the aten-maxpool2d-with-indices branch April 18, 2022 15:35
qedawkins pushed a commit to nod-ai/torch-mlir that referenced this pull request Oct 3, 2022
Co-authored-by: Kevin O'Brien <caomhin@us.ibm.com>
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.

5 participants