Skip to content

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