Uh oh!
There was an error while loading. Please reload this page.
Remove LoweredModule - #8886
Conversation
3fb6beb to
a6aa3b8Comparea6aa3b8 to
6ed813bCompareelectriclilies
commented
Sep 1, 2021
jroesch
commented
Sep 1, 2021
@manupa-arm@mbaret@Mousius could you guys take a review pass on this if you have cycles? should be a quick review, just consolidates everything into an IRModule. |
Mousius
left a comment
There was a problem hiding this comment.
Overall I'd say that this is a good change! I've put in a few suggestions for improvements but as this is evolving in the future I'm happy to see this merged and see if the code even exists in the next patch 😸
P.S. Thanks for addressing that nit @electriclilies 🥇 !
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| function_metadata_.Set(runtime::symbol::tvm_module_main, main_func_info.value()); | ||
| // Get only the Relay functions out of the lowered module so we can run type inference on them | ||
| IRModule main_module = tec::GetMainModule(lowered_mod); |
There was a problem hiding this comment.
Ok in a follow up if you like, but I think folding type inf into LowerTEPass makes sense to account for the rewritten calls. Someday we'll figure out how to gracefully do that incrementally since it's really overkill for just xfering known types from old to new CallNodes!
There was a problem hiding this comment.
I will add it as a TODO
There was a problem hiding this comment.
Actually there's a slight problem with this -- you can't run InferType on the whole lowered_mod becauselowered_mod has functions with GlobalVars that are in the IRModule's global_var_map, but you can't find them using Lookup because they are not the same object (it fails on line 210)..
https://github.com/apache/tvm/blob/main/src/relay/transforms/type_infer.cc#L209:L215
My workaround was to only apply type inferencing to the main_module (since that was what was done before this PR).
I'm not sure if this is a bug in how the type inferencer is dealing with GlobalVars (maybe it should be doing lookup by name hint, not pointer equality) or if it's a bug in how those GlobalVars are being made / propagated
There was a problem hiding this comment.
Then the other problem is even if you do successfully manage to lookup the function, there are PrimFuncs in the module which the type inferencer doesn't know how to deal with. We could just skip PrimFuncs we find during type inference..
Uh oh!
There was an error while loading. Please reload this page.
mbs-octoml
commented
Sep 2, 2021
LGTM. So much clearer, thank you Lily. |
electriclilies
commented
Sep 2, 2021
@Mousius@mbs-octoml Thanks for the review! |
leandron
commented
Sep 3, 2021
This is merged now, thanks @electriclilies@Mousius and @mbs-octoml! |
electriclilies
commented
Sep 3, 2021
thanks @leandron@Mousius@mbs-octoml! |
* Remove LoweredModule * Clean up some comments * QEMU flaky tests * Don't add external functions to the LoweredFunctions module * QEMU flaky test * Respond to feedback * flaky test
* Remove LoweredModule * Clean up some comments * QEMU flaky tests * Don't add external functions to the LoweredFunctions module * QEMU flaky test * Respond to feedback * flaky test
In this PR, I remove LoweredModule. I do this by annotating all functions with their targets and annotating external modules and function info on the module.
There is still some cleanup work to be done, specifically with removing per_target_modules_ in the interpreter. I'm going to remove those in a follow-up PR.
@mbs-octoml@mikepapadim@jroesch