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

[Bugfix] [tir] do not simplify 'Any() - Any()' to 0 #8266

Merged
merged 4 commits into from
Jul 17, 2021

Conversation

hgt312
Copy link
Contributor

@hgt312 hgt312 commented Jun 16, 2021

To reproduce

import tvm
a = tvm.tir.Any()
b = tvm.tir.Any()
c = tvm.tir.Sub(a, b)
tvm.arith.Analyzer().simplify(c)

With this change, tir considers a and b as different. However, structural_equal still consider them as the same.

@yzhliu @comaniac

Copy link
Contributor

@comaniac comaniac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Also cc @icemelon9 @junrushao1994 @Hzfengsy

@jroesch
Copy link
Member

jroesch commented Jun 16, 2021

Does it actually make sense to support this rule? semantically I'm not sure we guarantee that the same Any variable is equivalent.

For example:

a = any()
t1 = var(shape=(a,))
t2 = var(shape=(b,)) 

Conceptually I'm not sure: t1.shape == t2.shape at runtime.

@@ -293,6 +294,9 @@ def test_sub_index_simplify():

# mul co-efficient foldng
ck.verify(x - x, 0)
ck.verify(a - a, 0)
ck.verify(a - b, a - b)
ck.verify(a - b, c - c)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why could we verify a-b == c-c == 0? This doesn't make sense to me.

Copy link
Contributor Author

@hgt312 hgt312 Jun 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this line, a - b will not be simplified, it is still a - b, so structural_equal((a - b), (c-c)) is True (rhs will not be simplified in ck.verify). Just test the functionality of structural_equal

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not simplifying a - b is correct, but I think a - b is not structural equal to c - c as a and b refer to two different variable but c - c are the same variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this line now. Semantically, a - b is not equal to c - c, but structural_equal((a - b), (c-c)) works because for AnyNode it use the SEqualReduce function which only checks the node's dtype. In fact, a function contains AnyNode(s) in its ret_type after InferType will have new AnyNode(s), the two func then differs though they pass the tests.

I think a long term approach should be map Any like Var when checking the structures. So just keep it for now?

@hgt312
Copy link
Contributor Author

hgt312 commented Jun 17, 2021

Does it actually make sense to support this rule? semantically I'm not sure we guarantee that the same Any variable is equivalent.

For example:

a = any()
t1 = var(shape=(a,))
t2 = var(shape=(b,)) 

Conceptually I'm not sure: t1.shape == t2.shape at runtime.

Do you mean t2 = var(shape=(a,))?

IMHO, since the shapes share the same Any, they are equivalent.

@jcf94 jcf94 added the status: need update need update based on feedbacks label Jun 21, 2021
Copy link
Member

@icemelon icemelon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
@jroesch PTAL.

tests/python/unittest/test_arith_rewrite_simplify.py Outdated Show resolved Hide resolved
@junrushao
Copy link
Member

junrushao commented Jul 17, 2021

Thanks @hgt312 @icemelon9 @comaniac @MarisaKirisame! It is now merged

ylc pushed a commit to ylc/tvm that referenced this pull request Sep 29, 2021
* fix

* fix lint

* remove

* address comments
@hgt312 hgt312 deleted the fix_any branch December 12, 2021 01:45
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 13, 2022
* fix

* fix lint

* remove

* address comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: need update need update based on feedbacks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants