-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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] Pass manager #2546
[Relay] Pass manager #2546
Conversation
Let us spend a bit more time in discussing the Pass API as per https://docs.tvm.ai/contribute/code_review.html#deliberate-on-api-and-data-structures, I make some followup comments in the RFC |
@tqchen No problem, thanks. Let us discuss in the RFC. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall lgtm.
@tqchen fixed your comments, PTAL. Thanks. |
Not sure why quantized_pass failed. Seems it is not related to this PR. |
Some additional pass |
include/tvm/relay/pass.h
Outdated
typename = std::enable_if<(std::is_same<NodeT, Module>::value || | ||
std::is_same<NodeT, Function>::value)>> | ||
using PassFunc = | ||
runtime::TypedPackedFunc<runtime::TypedPackedFunc<NodeT(NodeT)>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do (NodeT, PassContext)->NodeT, so you do not capture the module, the outer pass will call this function to transform the corresponding module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor nit. General interface lgtm, please tag more reviewers
@wweic @jroesch @yzhliu @junrushao1994, @icemelon9 Please take a look when you have time. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. two additional doc comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait pass info change per #2512
@tqchen @jroesch @FrozenGene updated. Let's merge this and move on if it looks good to you guys. |
@tqchen Updated. Please take another looks. |
python/tvm/relay/ir_pass.py
Outdated
The callback function that sketches a certain optimization. | ||
""" | ||
|
||
def __init__(self, name, opt_level, pass_func, required=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if we really want to support constructor along with the construction function here. Perhaps we can just not define constructor and ask the user to use create the function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like it is probably necessary to leave it here so users have a flexibility to apply a pass_func directly without using decorators. I am okay with either way. I can remove it if you think it is unnecessary
python/tvm/relay/ir_pass.py
Outdated
disabled) | ||
|
||
|
||
def create_module_pass(pass_name, opt_level, pass_func, required=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to consider enable a decorator pattern here.
# Allow automatic derivation of pass name from the function name
@relay.ir_pass.module_pass(opt_level=2, required=[x, y, z])
def my_module_pass(mod, cx):
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
res0 = graph.evaluate(qgraph0)(dataset[0]['data']) | ||
res1 = graph.evaluate(qgraph1)(dataset[0]['data']) | ||
tvm.testing.assert_allclose(res0.asnumpy(), res1.asnumpy(), rtol=1e-3) | ||
# def test_quantize_pass(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try to keep these as it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We talked to Ziheng and this test is broken, he needs to fix it, he said he would open follow-up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think this pass has been also failing for me after I rebased it a while ago. @ZihengJiang said I can comment it out for now. He will look into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my additional comment on API design.
We should be really careful to make consistent argument orders between all APIs
python/tvm/relay/ir_pass.py
Outdated
disabled) | ||
|
||
|
||
def module_pass(opt_level, name=None, required=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consistency in the convention:
Let us make the argument list as follows:
def module_pass(pass_func=None, opt_level=None, name=None, required=None):
"""When pass_func == None, decorator mod, otherwise normal mode.
"""
Please also change all the constructors to keep the consistent order of arguments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API design is really important if we take a look at constructor order and the function order they are different.
This can be quite problematic and confuses user in the long run :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Using this signature, now I think we should remove those initialization functions.
python/tvm/relay/ir_pass.py
Outdated
return create_function_pass | ||
|
||
|
||
def sequential_pass(opt_level, passes, name="sequential_pass", required=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be consistent with other functions, argument order should be:
- passes
- name, can have a default value "sequential_pass"
- opt_level can have a default value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can find that this order is in particular inconsistent with others.
* initial commit * add python frontend and module tests * add unit tests for function pass and optimize interface * add ExprPass * remove PassState and pass context for run * add required_passes * return module * remove move * fix minor reviews * remove optimizer, optimizer->pass_manager, make pass a the base class of all * remove deleted files * move resolvedependency to sequential pass, use ir_pass namespace * add todo * add disabled passes in sequetialpass * fix minor * fix currying doc * remove pass_kind from passnode * remove pass kind from test * fix doc * fix per @tqchen's comments * remove pass_manager.py create separate classes * simplify pass_func * inline using passfunc * update doc * disable test_quantize_pass for now * create PassInfo class to contain the meta data * flatten passinfo for interface * retrigger ci * remove required method * make Pass python class lighter * create pass -> decorator * make the api consistent for all classes
* initial commit * add python frontend and module tests * add unit tests for function pass and optimize interface * add ExprPass * remove PassState and pass context for run * add required_passes * return module * remove move * fix minor reviews * remove optimizer, optimizer->pass_manager, make pass a the base class of all * remove deleted files * move resolvedependency to sequential pass, use ir_pass namespace * add todo * add disabled passes in sequetialpass * fix minor * fix currying doc * remove pass_kind from passnode * remove pass kind from test * fix doc * fix per @tqchen's comments * remove pass_manager.py create separate classes * simplify pass_func * inline using passfunc * update doc * disable test_quantize_pass for now * create PassInfo class to contain the meta data * flatten passinfo for interface * retrigger ci * remove required method * make Pass python class lighter * create pass -> decorator * make the api consistent for all classes
We have opened a RFC (#2512) for the pass manager. This PR implements the framework of it. Currently, we support the following type of passes:
Each pass works on a module. Passes could be registered in python and sent to c++. An
optimize
function is provided to accept a host of passes for analysis and optimization. The current PR only executes these passes without tuning the ordering.One example could be:
ret_mod
contains all the updated informationCC' @jroesch @wweic @ZihengJiang @tqchen @junrushao1994, @icemelon9, @FrozenGene, @MarisaKirisame please review. Thanks.