[Relay] fix exponential blowup in interpreter #3559
Conversation
|
I was thinking that the interpreter assumes ANF |
|
@junrushao1994 it assume anf, but most stuff pass in non-anf code. |
| second = InstantiateFuncType(ft2); | ||
| } | ||
| return solver_.Unify(first, second, expr); | ||
| return solver_.Unify(t1, t2, expr); |
There was a problem hiding this comment.
Just curious, why did you make this change? Did you incorporate that functionality in the unifier earlier?
There was a problem hiding this comment.
I need to enable let polymorphism as anf need let polymorphism to run on cps code as cps introduce polymorphic function because the answer type is polymorphic.
There was a problem hiding this comment.
Have you added unit tests for polymorphism in lets, then? It would be good to ensure that this change isn't introducing new bugs that way.
There was a problem hiding this comment.
Yes, the test is in test_cps. now it implicitly anf cps'd code, which introduce let polymorphism.
There was a problem hiding this comment.
I meant specific unit tests for these lets, but maybe those tests are sufficient. It would be good to have such unit tests (in test_type_infer) in case something breaks with that later and we want to pinpoint what's wrong
There was a problem hiding this comment.
test_let_polymorphism
|
Left some small comments but these seem to be good changes |
MarisaKirisame
force-pushed
the
feature
branch
2 times, most recently
from
b6e806e to
fa761c4
Compare
|
@MarisaKirisame please act on this PR. a possible solution might be only enable to anf before passing to interpreter, but not include it by default in other cases |
MarisaKirisame
force-pushed
the
feature
branch
from
fa761c4 to
82ad063
Compare
|
@slyubomirsky @tqchen I had addressed the review. |
|
@anijain2305 @SWu can you guys help review? |
|
@MarisaKirisame Thanks for the ping. I would have loved to review, but I don't think I am familiar with this codebase :-) But, let me add a couple of people who might be able to review - @icemelon9 , @wweic , @zhiics |
MarisaKirisame
force-pushed
the
feature
branch
from
3e0746e to
5607e35
Compare
|
@junrushao1994 @slyubomirsky can you guys give some review? |
| @@ -9,7 +9,7 @@ | |||
| # http://www.apache.org/licenses/LICENSE-2.0 | |||
| # | |||
| # Unless required by applicable law or agreed to in writing, | |||
| # software distributed under the License is distributed on an | |||
| # software distributed under the License is distributed on an1 | |||
There was a problem hiding this comment.
It is called emacs, lol
MarisaKirisame
force-pushed
the
feature
branch
from
e6f3009 to
f7fc132
Compare
|
@vinx13 @junrushao1994 can you guys give some review? |
|
|
||
| Type ret_type = Unify(ft1->ret_type, ft2->ret_type); | ||
|
|
||
| std::vector<Type> arg_types; | ||
| for (size_t i = 0; i < ft1->arg_types.size(); i++) { | ||
| for (size_t i = 0; i < ft2->arg_types.size(); ++i) { |
There was a problem hiding this comment.
what's the relation between ft1->arg_types.size() and ft2->arg_types.size()
There was a problem hiding this comment.
I imagine it has to do with the assumption "op->type_params.size() >= ftn->type_params.size()" (so ft2->arg_types.size() would be less than or equal to ft1->arg_types.size()). Is that right @MarisaKirisame ? There should probably be an additional comment.
|
@slyubomirsky Please review again and explicitly approve |
|
@vinx13 they can be of arbitrary size. we unify the excessive ones. |
|
I think the changes look good, but the func type unification rules changes probably merit more attention. @MarisaKirisame could you please give more explanation for the changes to the func type unification rules? (E.g., what problems do they solve? Should we have unit tests/regressions for those?) I'm trying to understand exactly what's going on there, since it's a pretty important piece of infrastructure |
|
@slyubomirsky it is a change that come with let polymorphism. this replace InstantiateFuncType while leaving as much type arguments as possible. |
|
I suppose it is probably a more principled approach than what we had before 👍 |
|
@slyubomirsky can you approve explicitly? |
previously, interpreter evaluate everything as tree form irregardless of graph sharing. This is bad because it cause exponential blowup:
a = 1
b = a + a
c = b + b
d = c + c
will be interpreter as ((1 + 1) + (1 + 1)) + ((1 + 1) + (1 + 1)) in the interpreter.
this fix use
featureto detect that there are no such graph sharing (or it will just failed).Additionally, it make executor anf before passing, and restore let polymorphism, as it is required to anf polymorphic code.
@icemelon9 @junrushao1994 @slyubomirsky @jroesch @vinx13 @tqchen can you guys help review?