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

[Relay] Mutual Recursion Support #5881

Closed
wants to merge 19 commits into from

Conversation

hypercubestart
Copy link
Contributor

Working on adding mutual recursion to as a relay pass.

cc for help and advice: @MarisaKirisame @jroesch

@jroesch
Copy link
Member

jroesch commented Aug 12, 2020

Sorry I meant to review this a long time ago and just got sidetracked by lots of things. I'm currently working on refactoring the type inferencer to use the new diagnostics. I should have a branch out by next week, could you potentially rebase on top of that and land these tests?

@hypercubestart
Copy link
Contributor Author

hypercubestart commented Aug 21, 2020

@hypercubestart hypercubestart changed the title [WIP][Relay] Mutual Recursion Support [Relay] Mutual Recursion Support Aug 21, 2020
@wweic
Copy link
Contributor

wweic commented Aug 23, 2020

I'll try to take a pass today

src/relay/analysis/type_solver.h Outdated Show resolved Hide resolved
src/relay/op/type_relations.cc Outdated Show resolved Hide resolved
src/relay/transforms/type_infer.cc Outdated Show resolved Hide resolved
src/relay/transforms/type_infer.cc Outdated Show resolved Hide resolved
tests/python/relay/test_type_infer.py Show resolved Hide resolved
@MarisaKirisame
Copy link
Contributor

Please rebase.

Copy link
Contributor

@MarisaKirisame MarisaKirisame left a comment

Choose a reason for hiding this comment

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

please remove the change to type relation, and fix/remove the test.

@hypercubestart
Copy link
Contributor Author

hypercubestart commented Aug 25, 2020

@wweic @jroesch could you review please?

@jroesch
Copy link
Member

jroesch commented Aug 25, 2020

Can we hold off on merging this until #6274 lands? This heavily changes the way the type inferencer works.

Copy link
Member

@jroesch jroesch left a comment

Choose a reason for hiding this comment

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

Let's put this on top of the new type inferencer changes in #6274

@MarisaKirisame
Copy link
Contributor

@hypercubestart sorry to put this on hold, but IMHO 6274 is an essential PR and we should focus on it with stronger priority. everything is good and we will merge this after 6274.

@@ -87,6 +87,10 @@ def _add(self, var, val, update=False):
var = _ty.GlobalTypeVar(var)
_ffi_api.Module_AddDef(self, var, val, update)

def add_unchecked(self, var, val):
Copy link
Contributor

Choose a reason for hiding this comment

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

add a doc string

@@ -98,6 +98,8 @@ def InferType():
"""
return _ffi_api.InferType()

def InferTypeAll():
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a doc string

@@ -65,6 +65,9 @@ class TypeSolver {
public:
TypeSolver(const GlobalVar& current_func, const IRModule& _mod, ErrorReporter* err_reporter);
~TypeSolver();

void SetCurrentFunc(GlobalVar current_func) { this->current_func = current_func; }
Copy link
Contributor

Choose a reason for hiding this comment

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

add a comment

}

void Solve();
Expr ResolveType(Expr expr);
Copy link
Contributor

Choose a reason for hiding this comment

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

add a comment.

@tqchen tqchen closed this Oct 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants