-
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] Relay annotation and partitioning for external compilers #4570
Conversation
@zhiics when I run test_pass_partition_graph.py, I get a warning which says
Is this expected? Or do I need to configure something on cmake? |
0c694d1
to
4c942c0
Compare
@masahi Sorry. I mistakenly changed the name of "ccompiler" directly. It therefore cannot find the registry and fallback to cpu. Now it is fixed. Please try it again. |
ok, fixed now. I have two questions regarding how this PR would interact with other transformation passes:
|
Yes, this is possible. I just added a unit test for this. Please check it out.
Yes, you should be able customize your opt pass pipeline. For example: # Given a relay module, mod
with relay.quantize.qconfig(...):
q_mod = relay.quantize.quantize(mod, params)
seq1 = [relay.transform.xx, relay.transform.yy]
with relay.build_config(...)
mod = seq1(q_mod)
mod = relay.build_extern_compiler(mod, "ccompiler")
relaly.build_config(...):
graph, lib, params = relay.build(mod, "llvm") |
@zhiics great, thanks. |
@zhiics When I run the mobilenet test and dump the annotated graph, batch norm is not converted to dnnl one.
It is probably because batch norm is decomposed during SimplifyInference. Is batch norm supposed to be around during external codegen time? I think it should be. I don't hit "else if (IsOp(call, "nn.batch_norm")" condition here during mobilenet test. I want to see an example of matching a pattern like Conv + BN + Relu and translate them to dnnl's fused op. It would be great if you could do that in this or future PR, otherwise I'll do it as a practice and send a PR :) |
The BN op should be turned off in DNNL codegen because we haven't supported multiple outputs yet.
The BN op will not be decomposed if it is marked as an external op.
|
If BN is not decomposed, why am I not seeing dnnl_bn generated? |
It is not marked as external because |
@comaniac ok thanks, I'll study the code. |
BTW, the |
Ideally whether to keep batch norm or not should be configurable. Actually for my use case, I want batch norm to be decomposed and its multiplication folded into previous conv, so that I don't have to worry about supporting batch norm on my end. But I expect others have a special op for handling fused Conv + BN + Relu. UPDATE: I realized this is already achieved by allowing each backend to have its own annotate_compiler registration. |
Yes you are right. This PR provides an interface (annotation) for specialized codegen to do subgraph pattern matching for fusion or similar optimization. However, we also realize that this may be painful for developers and it might be better to provide a more convenient pattern matching for widely used cases (e.g., Conv+BN+ReLU as you mentioned). Our follow-up plans to focus more on the graph partitioning algorithm, and this is one of the cases we want to cover. |
For others who might be interested in this PR, here is a before/after of partitioning in test_multi_node_compiler(). Before
After
|
23154bd
to
d502341
Compare
It would be great to think a bit more about the API. Personally, I don't like the fact that we are adding new build functions (build_extern_compiler). We should advocate the pipeline API more and avoid the general interface. Given that this is not our final form, I would suggest we only keep the pipeline API. e.g. mod = relay.transform.AnnotateExternCompiler("dnnl")(mod) Similarly, the term register_annotate_compiler seems to be quite arbitrary. Is it mainly built for dnnl? should it have a dnnl namespace instead, should we just make use of set_attr for now? |
So to summary my concerns. Extern compilation support is definitely an important feature, how to design the related API will have a quite big impact for our users. While I think the related PRs have the techincal solution to deliver "a solution" for our problem. It would be really nice if we can bring a few API design options(e.g. choices of build_extern_compiler vs AnnotateCompiler, the choice of register_annotate_compiler, their signatures) as RFC discussion, this way we can make sure we design the most easy to use APIs for our developers. How about open a new discuss thread that contains those choices and try to get people's opinion on the choices? |
d502341
to
01c5f2c
Compare
As discussed in https://discuss.tvm.ai/t/rfc-naming-and-api-signature-for-external-compilation/5292, let's remove the annotation template for now. Instead, we only focus on partitioning and expect developers to annotate the program first. Some example annotators that leverage the pass manager are added to help developers write their own annotator. Followup PRs will be sent to help annotation which may need more discussion and thoughts on how to combine together with OpStrategy. |
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
01c5f2c
to
703ab73
Compare
@tqchen We removed the annotation part, any other outstanding concerns? |
Co-Authored-By: 雾雨魔理沙 <lolisa@marisa.moe>
703ab73
to
2425cac
Compare
…che#4570) * [relay] Relay annotation and partitioning for codegen * Add fusion unit test * fix comments * Update include/tvm/relay/attrs/annotation.h Co-Authored-By: 雾雨魔理沙 <lolisa@marisa.moe> * rebase * remove annotation helper * rebase again Co-authored-by: Cody Yu <comaniac0422@gmail.com> Co-authored-by: 雾雨魔理沙 <lolisa@marisa.moe>
…che#4570) * [relay] Relay annotation and partitioning for codegen * Add fusion unit test * fix comments * Update include/tvm/relay/attrs/annotation.h Co-Authored-By: 雾雨魔理沙 <lolisa@marisa.moe> * rebase * remove annotation helper * rebase again Co-authored-by: Cody Yu <comaniac0422@gmail.com> Co-authored-by: 雾雨魔理沙 <lolisa@marisa.moe>
…che#4570) * [relay] Relay annotation and partitioning for codegen * Add fusion unit test * fix comments * Update include/tvm/relay/attrs/annotation.h Co-Authored-By: 雾雨魔理沙 <lolisa@marisa.moe> * rebase * remove annotation helper * rebase again Co-authored-by: Cody Yu <comaniac0422@gmail.com> Co-authored-by: 雾雨魔理沙 <lolisa@marisa.moe>
This is the partitioning part of #4258
Updated
Here, we only focus on partitioning and expect developers to annotate the program first. We provide some examples to help developers to write their own annotators using pass manager directly from Python frontend.
How to use
Followup PRs will be sent to:
provide a convenient annotation framework which may need more discussion and thoughts on how to combine together with OpStrategy
tutorials to help developers add external codegen/runtime and create their own build pipeline.
add more comprehensive operator support for dnnl
CC @tqchen @jroesch @icemelon9 @u99127 @masahi @u99127 @soiferj @comaniac