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] Pass manager #2546

Merged
merged 32 commits into from
Mar 12, 2019
Merged

[Relay] Pass manager #2546

merged 32 commits into from
Mar 12, 2019

Conversation

zhiics
Copy link
Member

@zhiics zhiics commented Feb 2, 2019

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:

  • module-level passes, they mainly focuses on optimizations cross functions, like inter-procedural optimizations/analysis, addition and removal of functions.
  • function-level passes, they focuses on optimizations in a relay function scope

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:

class MyClass():
    def __init__(self, mod):
        self.module = mod

    def transform(node, pass_ctx):
        """Perform optimization on node."""
        pass

my_class = MyClass(mod)
def transform(expr, pass_ctx):
    return my_class.transform(expr, pass_ctx)

module_pass = ModulePass("module_pass", 1, transform)
function_pass = FunctionPass("function_pass", 2, transform)

mod = relay.Module(...)
passes = [module_pass, function_pass]
sequential_pass = pass_manager.SequentialPass("sequential_pass", 2, passes)
ret_mod = sequential_pass(mod)

ret_mod contains all the updated information

CC' @jroesch @wweic @ZihengJiang @tqchen @junrushao1994, @icemelon9, @FrozenGene, @MarisaKirisame please review. Thanks.

@zhiics zhiics changed the title Pass manager [Relay] Pass manager Feb 2, 2019
@tqchen
Copy link
Member

tqchen commented Feb 2, 2019

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

@zhiics
Copy link
Member Author

zhiics commented Feb 2, 2019

@tqchen No problem, thanks. Let us discuss in the RFC.

include/tvm/relay/optimizer.h Outdated Show resolved Hide resolved
src/relay/ir/module.cc Outdated Show resolved Hide resolved
tests/python/relay/test_pass_optimizer.py Outdated Show resolved Hide resolved
Copy link
Contributor

@wweic wweic left a comment

Choose a reason for hiding this comment

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

overall lgtm.

include/tvm/relay/optimizer.h Outdated Show resolved Hide resolved
@tqchen tqchen self-assigned this Feb 19, 2019
include/tvm/relay/pass_manager.h Outdated Show resolved Hide resolved
include/tvm/relay/pass_manager.h Outdated Show resolved Hide resolved
include/tvm/relay/pass_manager.h Outdated Show resolved Hide resolved
include/tvm/relay/pass_manager.h Outdated Show resolved Hide resolved
include/tvm/relay/pass_manager.h Outdated Show resolved Hide resolved
include/tvm/relay/pass_manager.h Outdated Show resolved Hide resolved
include/tvm/relay/pass_manager.h Outdated Show resolved Hide resolved
include/tvm/relay/pass_manager.h Outdated Show resolved Hide resolved
include/tvm/relay/pass_manager.h Outdated Show resolved Hide resolved
include/tvm/relay/pass_manager.h Outdated Show resolved Hide resolved
include/tvm/relay/pass_manager.h Outdated Show resolved Hide resolved
include/tvm/relay/pass_manager.h Outdated Show resolved Hide resolved
include/tvm/relay/pass_manager.h Outdated Show resolved Hide resolved
include/tvm/relay/pass_manager.h Outdated Show resolved Hide resolved
python/tvm/relay/_pass_manager.pyi Outdated Show resolved Hide resolved
python/tvm/relay/pass_manager.py Outdated Show resolved Hide resolved
tests/python/relay/test_pass_manager.py Outdated Show resolved Hide resolved
@tqchen
Copy link
Member

tqchen commented Mar 1, 2019

@zhiics @jroesch I made some further comments, mainly on minimum interface design, please check

include/tvm/relay/pass_manager.h Outdated Show resolved Hide resolved
include/tvm/relay/pass_manager.h Outdated Show resolved Hide resolved
@zhiics
Copy link
Member Author

zhiics commented Mar 2, 2019

@tqchen fixed your comments, PTAL. Thanks.

@zhiics
Copy link
Member Author

zhiics commented Mar 2, 2019

Not sure why quantized_pass failed. Seems it is not related to this PR.

include/tvm/relay/pass.h Outdated Show resolved Hide resolved
include/tvm/relay/pass.h Outdated Show resolved Hide resolved
include/tvm/relay/pass.h Outdated Show resolved Hide resolved
include/tvm/relay/pass.h Outdated Show resolved Hide resolved
include/tvm/relay/pass.h Outdated Show resolved Hide resolved
include/tvm/relay/pass.h Show resolved Hide resolved
include/tvm/relay/pass.h Outdated Show resolved Hide resolved
python/tvm/relay/pass_manager.py Outdated Show resolved Hide resolved
python/tvm/relay/pass_manager.py Outdated Show resolved Hide resolved
python/tvm/relay/pass_manager.py Outdated Show resolved Hide resolved
@tqchen
Copy link
Member

tqchen commented Mar 2, 2019

Some additional pass

include/tvm/relay/pass.h Outdated Show resolved Hide resolved
typename = std::enable_if<(std::is_same<NodeT, Module>::value ||
std::is_same<NodeT, Function>::value)>>
using PassFunc =
runtime::TypedPackedFunc<runtime::TypedPackedFunc<NodeT(NodeT)>(
Copy link
Member

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.

Copy link
Member

@tqchen tqchen left a 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

include/tvm/relay/pass.h Outdated Show resolved Hide resolved
@zhiics
Copy link
Member Author

zhiics commented Mar 3, 2019

@wweic @jroesch @yzhliu @junrushao1994, @icemelon9 Please take a look when you have time. Thanks.

include/tvm/relay/pass.h Outdated Show resolved Hide resolved
python/tvm/relay/ir_pass.py Outdated Show resolved Hide resolved
python/tvm/relay/ir_pass.py Outdated Show resolved Hide resolved
python/tvm/relay/ir_pass.py Outdated Show resolved Hide resolved
python/tvm/relay/ir_pass.py Outdated Show resolved Hide resolved
python/tvm/relay/ir_pass.py Outdated Show resolved Hide resolved
src/relay/pass/pass_manager.cc Outdated Show resolved Hide resolved
include/tvm/relay/pass.h Outdated Show resolved Hide resolved
include/tvm/relay/pass.h Outdated Show resolved Hide resolved
Copy link
Contributor

@wweic wweic left a 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.

python/tvm/relay/ir_pass.py Outdated Show resolved Hide resolved
python/tvm/relay/ir_pass.py Outdated Show resolved Hide resolved
Copy link
Member

@tqchen tqchen left a 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

@zhiics
Copy link
Member Author

zhiics commented Mar 8, 2019

@tqchen @jroesch @FrozenGene updated. Let's merge this and move on if it looks good to you guys.

python/tvm/relay/ir_pass.py Outdated Show resolved Hide resolved
include/tvm/relay/pass.h Outdated Show resolved Hide resolved
python/tvm/relay/ir_pass.py Outdated Show resolved Hide resolved
python/tvm/relay/ir_pass.py Outdated Show resolved Hide resolved
python/tvm/relay/ir_pass.py Show resolved Hide resolved
@zhiics
Copy link
Member Author

zhiics commented Mar 10, 2019

@tqchen Updated. Please take another looks.

python/tvm/relay/ir_pass.py Outdated Show resolved Hide resolved
The callback function that sketches a certain optimization.
"""

def __init__(self, name, opt_level, pass_func, required=None):
Copy link
Member

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

Copy link
Member Author

@zhiics zhiics Mar 11, 2019

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

disabled)


def create_module_pass(pass_name, opt_level, pass_func, required=None):
Copy link
Member

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):
     ...

Copy link
Member Author

Choose a reason for hiding this comment

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

changed

src/relay/pass/pass_manager.cc Outdated Show resolved Hide resolved
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():
Copy link
Member

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.

Copy link
Member

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.

Copy link
Member Author

@zhiics zhiics Mar 11, 2019

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.

Copy link
Member

@tqchen tqchen left a 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

disabled)


def module_pass(opt_level, name=None, required=None):
Copy link
Member

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

Copy link
Member

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 :)

Copy link
Member Author

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.

return create_function_pass


def sequential_pass(opt_level, passes, name="sequential_pass", required=None,
Copy link
Member

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.

Copy link
Member

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.

python/tvm/relay/ir_pass.py Show resolved Hide resolved
@jroesch jroesch merged commit abe6f77 into apache:master Mar 12, 2019
@zhiics zhiics deleted the pass_manager branch March 12, 2019 03:01
wweic pushed a commit to wweic/tvm that referenced this pull request Mar 12, 2019
* 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
wweic pushed a commit to neo-ai/tvm that referenced this pull request Mar 13, 2019
* 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
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.

8 participants