Skip to content

Conversation

@MarisaKirisame
Copy link
Contributor

Thanks for contributing to TVM! Please refer to guideline https://docs.tvm.ai/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from Reviewers.

// slow path with path compression.
TypeNode* root = this;
while (root->parent != nullptr) {
CHECK(root->parent != root) << "should not refer back to itself";
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not return root if you detect root->parent == root? This way, you won't throw (unnecessary?) exception (BTW the new unit test failed). The following snippet seems normal program to me and we probably should support it?

"""Program:
       def f(x : Tensor[(10, 10), f32]) {
           return log(x);
       }
    """

from tvm import relay
a = relay.TypeVar("a")
x = relay.var("x", a)
sb = relay.ScopeBuilder()
f = relay.Function([x], x)
fx = relay.Call(f, [x])
assert relay.ir_pass.infer_type(x).checked_type == a
assert relay.ir_pass.infer_type(f).checked_type == relay.FuncType([a], a)
assert relay.ir_pass.infer_type(fx).checked_type == a

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I did, and it is breaking invariant somewhere else (so did not get merged).
see #1983

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is supposed to fail. This is more of a issue (but issue wont get ci).

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh OK, I'm interested in how such self reference structure is created cause the test case is trivial. FYI, I tried my fix and it correctly pass the above tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's good. I have no time to investigate into type inference internal right now, so we might have this on hold until I have more time slot (after 20days). IDK.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually for this particular case, it's simply because we don't check type equality before we do unification(and we should always do the quality check before we try to unify them):
https://github.com/dmlc/tvm/blob/cbf4fdbbbf4993849342906722e776d332f2e97d/src/relay/pass/type_solver.cc#L71

I have a test test that failed master and works after the fix. Could you help review?

Copy link
Contributor Author

@MarisaKirisame MarisaKirisame Oct 26, 2018

Choose a reason for hiding this comment

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

Yes!

Copy link
Member

@jroesch jroesch Oct 26, 2018

Choose a reason for hiding this comment

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

This problem is due to there not being an occurs check: https://en.wikipedia.org/wiki/Occurs_check

Copy link
Contributor

Choose a reason for hiding this comment

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

PR: #2012 welcome to take a look :-)

@tqchen
Copy link
Member

tqchen commented Oct 27, 2018

fixed by #2012

@tqchen tqchen closed this Oct 27, 2018
@MarisaKirisame MarisaKirisame deleted the bug branch October 27, 2018 03:42
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.

4 participants