-
Notifications
You must be signed in to change notification settings - Fork 496
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
[MHLO] Add configurable op decomposition rules #1172
Conversation
Co-authored-by: Bairen Yi <yibairen.byron@bytedance.com> Co-authored-by: Jiawei Wu <xremold@gmail.com> Co-authored-by: Tianyou Guo <tianyou.gty@alibaba-inc.com> Co-authored-by: Xu Yan <yancey.yx@alibaba-inc.com> Co-authored-by: Ziheng Jiang <ziheng.jiang@bytedance.com>
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
Can we add some file check unit tests for the pass? |
Done. |
*this, "torch-decomposition-skip-ops", | ||
llvm::cl::desc("Disable decomposition rules for these torch ops. Useless " | ||
"when decompose-complex-ops set to false."), | ||
llvm::cl::init(",")}; |
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.
Can we change the default value to ""
simply?
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.
Great work!
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 have some specific opinions about how we want to do this -- I am on vacation the rest of this week, but after #1165 lands this was the next thing I was going to work on (and this PR creates a nasty merge conflict). If you can wait for a week or so I would be happy to implement this.
|
||
// Disable decomposition rules for torch ops. Useless when | ||
// decompose-complex-ops set to false. | ||
Option<std::string> torchDecompositionSkipOps{ |
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.
This can be a list<string>
I think. And it should not have torch
in the name.
// If this option is false, decompose complex operations. | ||
// If this option is true, skip decomposition of complex operations. | ||
Option<bool> decompose{*this, "decompose-complex-ops", llvm::cl::desc("Decompose complex operations."), | ||
llvm::cl::init(true)}; | ||
Option<bool> decompose{*this, "decompose-complex-ops", |
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 would prefer that we have something a bit more principled here. Like one option which is an allowlist and another which is a denylist so users can control it more precisely.
That's okay. I can wait for that. |
There is a better choice in PR #1242 . This PR is useless and closed. |
Signed-off-by: Ettore Tiotto <etiotto@ca.ibm.com> Co-authored-by: Alexandre Eichenberger <alexe@us.ibm.com>
See RFC #999
We register some options in
TorchLoweringPipeline
andDecomposeComplexOpsPass
to make the decomposition rules configurable.Co-authored-by: Bairen Yi yibairen.byron@bytedance.com
Co-authored-by: Jiawei Wu xremold@gmail.com
Co-authored-by: Tianyou Guo tianyou.gty@alibaba-inc.com
Co-authored-by: Xu Yan yancey.yx@alibaba-inc.com
Co-authored-by: Ziheng Jiang ziheng.jiang@bytedance.com