-
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
[DNNL][BYOC] Enable Altering Dense Weight Layout #11966
base: main
Are you sure you want to change the base?
Conversation
@apeskov @masahi @yangulei @crazydemo @linlifan @Qianshui-Jiang Please take a look :-) |
@@ -94,6 +94,9 @@ def partition_for_dnnl(mod, params=None, alter_layout=True, prune_subgraphs=True | |||
) | |||
with tvm.transform.PassContext(opt_level=3): | |||
mod = seq(mod) | |||
|
|||
mod = dnnl.rewrite_dense_bias_gelu_reshape_last(mod) |
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.
Could you please explain, why do we need this pass before "AlterOp" transformation passes?
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.
Hi Peskov, This pass rewrite_dense_bias_gelu_reshape_last only matches the pattern "dense_bias_activation". If we are desired to put it after "AlterOp" pass, then I need to extend the capability of pass rewrite_dense_bias_gelu_reshape .
Thanks @billishyahao, nice patch! You've touched a common tvm::relay code a little bit to enhance layouts support of packed dense op. There is one delicate nuance here and I would like to highlight it. "weight_layout" is arbitrary string like "NC", "CN", "CN8c", "CN16n4c" and any others. It should match with regex: So I recommend you to take into account |
Hi @apeskov, what you mentioned is a common issue of blocked layout.
If additional Padding is natural, while Maybe we need a Pass to Infer the original logical shapes and save them as attributes for later usage, do you have any idea about this? |
29dd2bd
to
9c6a910
Compare
86fea87
to
a81ca9c
Compare
Hi @masahi , Could you shed some light on the ci failure? I could not find a way to reproduce it on local environment. Thanks! |
sorry it's unclear from the logs, we really should aggregate common error phrases automatically. If you search through the logs for
) |
@tvm-bot rerun |
Hi @masahi , Thanks for quick response. I found another testcase failed in https://ci.tlcpack.ai/blue/rest/organizations/jenkins/pipelines/tvm/branches/PR-11966/runs/11/nodes/382/steps/1159/log/?start=0. Is it a random issue? |
Yeah that looks also unrelated and flaky. It's strange, I did a PR yesterday and met none of these issues. #12263 |
@tvm-bot rerun |
The patch including four parts:
Thanks for contributing to TVM! Please refer to guideline https://tvm.apache.org/docs/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from Reviewers by @ them in the pull request thread.