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

API Compat fix on max_pool2d_with_indices_() #65

Closed
wants to merge 29 commits into from
Closed

Conversation

brucekimrokcmu
Copy link
Contributor

This PR addresses the following error to resolve a number of test cases:

1 : MaxPool2dWithIndicesFullSizeKernelModule_basic
max_pool2d_with_indices(): incompatible function arguments. The following argument types are supported:
    1. (self: pi.mlir._mlir_libs._pi_mlir.Tensor, kernel_size: pi.mlir._mlir_libs._pi_mlir.AnyTorchListOfTorchIntValue, stride: pi.mlir._mlir_libs._pi_mlir.AnyTorchListOfTorchIntValue = [], padding: pi.mlir._mlir_libs._pi_mlir.AnyTorchListOfTorchIntValue = [0, 0], dilation: pi.mlir._mlir_libs._pi_mlir.AnyTorchListOfTorchIntValue = [1, 1], ceil_mode: pi.mlir._mlir_libs._pi_mlir.Torch_BoolValue = False, *, loc: mlir.ir.Location = None, ip: mlir.ir.InsertionPoint = None) -> Tuple[pi.mlir._mlir_libs._pi_mlir.Tensor, pi.mlir._mlir_libs._pi_mlir.Tensor]

Invoked with: Tensor(<block argument> of type '!torch.tensor' at index: 0); kwargs: kernel_size=[Torch_IntValue(%0 = "torch.constant.int"() {value = 4 : i64} : () -> !torch.int), Torch_IntValue(%1 = "torch.constant.int"() {value = 4 : i64} : () -> !torch.int)], stride=1, padding=0, dilation=1 

Essentially, a recursive template function generates 16 different combinations ofPyAnyTorchListOfTorchIntValue and PyTorch_IntValue for four arguments (kernel_size, stride, padding, dilation) to be accepted for max_pool2d_with_indices_ Ops.

Note that to avoid segfault, max_pool2d_with_indices() redefines default loc and ip before all casted arguments are passed into max_pool2d_with_indices_() ops function.

@123epsilon
Copy link
Contributor

The purpose of this PR is to address several test cases for max_pool2d_with_indices that require accepting different combinations of a list versus an int for parameters such as kernel_size. Since there are a lot of combinations (16 for 4 parameters) we generate the appropriate bindings via recursive template instantiation. There is one downside in the current implementation: we do not have default arguments for the arguments to the Op.

This passes an additional 6 tests

@makslevental
Copy link
Collaborator

makslevental commented Jul 19, 2023

the red x here is because of whate we discussed offline? missing torch-mlir wheel?

edit: indeed it is

@brucekimrokcmu
Copy link
Contributor Author

Allowed max_pool2d and max_pool2d_with_indices to accept kwargs with default values to let users omit some of the kwargs in use. This passed one more test case, but essentially allowed freedom in passing kwargs or not.

@123epsilon
Copy link
Contributor

@brucekimrokcmu it seems like you rebased against main but your PR is trying to change the torch-mlir submodule, can you verify whether your local branch is at the most recent torch-mlir commit in externals/torch-mlir it should be 1e468e8, the commit history looks a little strange to me

123epsilon and others added 3 commits July 23, 2023 20:47
Co-authored-by: Arham Khan <arhamkhan@Arhams-MacBook-Pro.local>
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.

3 participants