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

[TOPI][Bugfix] Make semantics of empty axis in squeeze consistent with Relay #12596

Merged
merged 3 commits into from
Aug 26, 2022
Merged

[TOPI][Bugfix] Make semantics of empty axis in squeeze consistent with Relay #12596

merged 3 commits into from
Aug 26, 2022

Conversation

wzh99
Copy link
Contributor

@wzh99 wzh99 commented Aug 25, 2022

This PR fixes #11697. The analysis of the bug is elaborated in #12400. The key problem is whether axis in squeeze means doing nothing or squeezing all one-dimensional axes.

In #12400, I provide two possible fixes. I choose to fix TOPI finally. The meaning of axis is clearly described here (commit #2020):

TVM_DECLARE_ATTRS(SqueezeAttrs, "relay.attrs.SqueezeAttrs") {
TVM_ATTR_FIELD(axis)
.describe(
"The axis to squeeze in the input tensor."
"If `axis = None`, all axis of dimension 1 get squeezed;"
"Else, the dimension in axes get squeezed."
"It is an error if an axis does not has dimension 1.")
.set_default(NullValue<Array<Integer>>());
}

The later commit to TOPI (#2147), however, does not follow this description. It seems that there is a mistake in this commit and should be corrected.

CC: @masahi

@masahi
Copy link
Member

masahi commented Aug 25, 2022

Can you add a test?

@wzh99
Copy link
Contributor Author

wzh99 commented Aug 26, 2022

@masahi Of course. It seems that I should add the test here:

@tvm.testing.requires_gpu
def test_squeeze():
verify_squeeze((1, 2, 3, 4), 0)
verify_squeeze((1, 2, 1, 4), None)
verify_squeeze((1, 1, 1, 4), (1, 2))
verify_squeeze((1, 1, 1, 1), None)

I have prepared the test case verify_squeeze((1, 1, 1, 1), ()) to see whether TOPI correctly handles empty axis. However, I have a problem with this testing function before pushing the commit: Why is it decorated with @tvm.testing.requires_gpu, instead of @tvm.testing.uses_gpu as the other testing functions in this file? In other words, can it run on a CPU-only target?

@masahi
Copy link
Member

masahi commented Aug 26, 2022

hmm it's weird that squeeze is only tested on gpu targets:

for target in ["cuda", "opencl"]:

Can you add llvm there and replace requires_gpu with uses_gpu?

@wzh99
Copy link
Contributor Author

wzh99 commented Aug 26, 2022

@masahi I have pushed the commits, including the new test case and llvm target.

@masahi masahi merged commit 3224817 into apache:main Aug 26, 2022
@wzh99 wzh99 deleted the squeeze-axis branch August 26, 2022 07:29
xinetzone pushed a commit to daobook/tvm that referenced this pull request Nov 25, 2022
… with Relay (apache#12596)

* Fix empty axis of `squeeze` in TOPI.

* Add test case for `squeeze` with empty `axis`.

* Add LLVM target for `test_squeeze`.
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.

[Bug] Execution error of a simple nn.bias_add + squeeze graph where axis=[]
2 participants