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

inductor: rewrite mkldnn fx fusion using pattern_matcher(conv_unary) #97007

Closed

Conversation

…onv_unary)"

cc soumith voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 desertfire

[ghstack-poisoned]
…onv_unary)"

cc soumith voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 desertfire

[ghstack-poisoned]
@pytorch-bot
Copy link

pytorch-bot bot commented Mar 17, 2023

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/97007

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 186a588:
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

XiaobingSuper added a commit that referenced this pull request Mar 17, 2023
ghstack-source-id: 779777fad1d385b53a45524b12c3401f787277d5
Pull Request resolved: #97007
XiaobingSuper added a commit that referenced this pull request Mar 17, 2023
ghstack-source-id: ae74b1f32376fe8941ea8c8e3e01e3f0f45bd470
Pull Request resolved: #97007
@XiaobingSuper XiaobingSuper marked this pull request as draft March 17, 2023 07:35
XiaobingSuper added a commit that referenced this pull request Mar 17, 2023
ghstack-source-id: e5f15609e22d0171b3945f0f27aff7039170815e
Pull Request resolved: #97007
…onv_unary)"

cc soumith voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 desertfire

[ghstack-poisoned]
XiaobingSuper added a commit that referenced this pull request Mar 20, 2023
ghstack-source-id: b6ab144c346a478f87e8f7b77cf35ea55ea05500
Pull Request resolved: #97007
…onv_unary)"

cc soumith voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 desertfire

[ghstack-poisoned]
XiaobingSuper added a commit that referenced this pull request Mar 20, 2023
ghstack-source-id: 9484d4056c62d3cd1cc3c0e501c4410977f589ee
Pull Request resolved: #97007
Copy link
Collaborator

@jgong5 jgong5 left a comment

Choose a reason for hiding this comment

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

LGTM.

torch/_inductor/pattern_matcher.py Outdated Show resolved Hide resolved
@@ -138,7 +130,7 @@ def _update_module_params(self, conv, unary, input_size):

def _conv_forward(self, input, weight, bias):
if self.padding_mode != "zeros":
return torch.ops.mkldnn._convolution_pointwise(
return torch.mkldnn_convolution(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's add a single conv and linear for the repacked conv. The semantic of _conv_pointwise is confusing as it serves both the single conv and the fused conv.

torch/_inductor/pattern_matcher.py Outdated Show resolved Hide resolved
Comment on lines 803 to 805
return L[aten.clamp_max](
L[aten.clamp_min](conv_out, min_value), max_value
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the decompose version of hardtanh, right? Is there any way to call the decomposed function directly but not duplicate it again.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't find a way to do it.

…onv_unary)"

cc soumith voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 desertfire

[ghstack-poisoned]
…onv_unary)"

cc jgong5 mingfeima sanchitintel ashokei jingxu10 soumith voznesenskym penguinwu anijain2305 EikanWang Guobing-Chen zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 desertfire

[ghstack-poisoned]
…onv_unary)"

cc jgong5 mingfeima sanchitintel ashokei jingxu10 soumith voznesenskym penguinwu anijain2305 EikanWang Guobing-Chen zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 desertfire

[ghstack-poisoned]
@pytorchmergebot
Copy link
Collaborator

Rebased gh/XiaobingSuper/84/orig onto refs/remotes/origin/viable/strict because #97141 was rebased, please pull locally before adding more changes (for example, via ghstack checkout https://github.com/pytorch/pytorch/pull/97007)

…onv_unary)"

cc jgong5 mingfeima sanchitintel ashokei jingxu10 soumith voznesenskym penguinwu anijain2305 EikanWang Guobing-Chen zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 desertfire

[ghstack-poisoned]
…onv_unary)"

cc jgong5 mingfeima sanchitintel ashokei jingxu10 soumith voznesenskym penguinwu anijain2305 EikanWang Guobing-Chen zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 desertfire

[ghstack-poisoned]
@@ -608,6 +608,279 @@ def addmm(match, mat1, mat2, inp):
return L[aten.add](inp, L[aten.mm](mat1, mat2))


# TODO(XiaobingSuper): move it to fx_passes/mkldnn_fuse.py
Copy link
Contributor

Choose a reason for hiding this comment

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

I am ok with this stack of changes, but please move them into a separate file.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can create a fx_passes/ folder to match with #97741

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, move it to a separate file.

Comment on lines 722 to 729
def relu_fusion(computation_call):
return CallFunction(aten.relu, computation_call)

def sigmoid_fusion(computation_call):
return CallFunction(aten.sigmoid, computation_call)

def tanh_fusion(computation_call):
return CallFunction(aten.tanh, computation_call)
Copy link
Contributor

Choose a reason for hiding this comment

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

You can match multiple functions in a single pattern like:

Suggested change
def relu_fusion(computation_call):
return CallFunction(aten.relu, computation_call)
def sigmoid_fusion(computation_call):
return CallFunction(aten.sigmoid, computation_call)
def tanh_fusion(computation_call):
return CallFunction(aten.tanh, computation_call)
def combined_fusion(computation_call):
return CallFunction([aten.relu, aten.sigmoid, aten.tanh], computation_call)

Then figure out which one you hit with the match arg to the lowering.

This should allow you to have fewer patterns throughout the stack.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, it is simplified.

…onv_unary)"

cc jgong5 mingfeima sanchitintel ashokei jingxu10 soumith voznesenskym penguinwu anijain2305 EikanWang Guobing-Chen zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 desertfire

[ghstack-poisoned]
…onv_unary)"

cc jgong5 mingfeima sanchitintel ashokei jingxu10 soumith voznesenskym penguinwu anijain2305 EikanWang Guobing-Chen zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 desertfire

[ghstack-poisoned]
…onv_unary)"

cc jgong5 mingfeima sanchitintel ashokei jingxu10 soumith voznesenskym penguinwu anijain2305 EikanWang Guobing-Chen zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 desertfire

[ghstack-poisoned]
@XiaobingSuper
Copy link
Collaborator Author

@jansel @desertfire, please help review this code again. Thanks!

…onv_unary)"

cc jgong5 mingfeima sanchitintel ashokei jingxu10 soumith voznesenskym penguinwu anijain2305 EikanWang Guobing-Chen zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 desertfire

[ghstack-poisoned]
for pattern, computation_op in zip(_hardtanh_patterns, computation_ops):
_register_hardtanh_fusion_lowering(pattern, computation_op)

def _mkldnn_fusion_init():
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def _mkldnn_fusion_init():
@functools.lru_cache(None)
def _mkldnn_fusion_init():

So this code only runs once.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed.

Comment on lines 42 to 45
if torch._C.has_mkldnn:
from .mkldnn_fusion import _mkldnn_fusion_init

_mkldnn_fusion_init()
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this code into a lazy_init() function with an lru_cache(None) decorator so it only runs once.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved the code to lazy_init(). Thanks!

…onv_unary)"

cc jgong5 mingfeima sanchitintel ashokei jingxu10 soumith voznesenskym penguinwu anijain2305 EikanWang Guobing-Chen zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 desertfire

[ghstack-poisoned]
@XiaobingSuper XiaobingSuper added the ciflow/trunk Trigger trunk jobs on your pull request label Apr 11, 2023
…onv_unary)"

cc jgong5 mingfeima sanchitintel ashokei jingxu10 soumith voznesenskym penguinwu anijain2305 EikanWang Guobing-Chen zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 desertfire

[ghstack-poisoned]
@XiaobingSuper
Copy link
Collaborator Author

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: Command git -C /home/runner/work/pytorch/pytorch cherry-pick -x ae89899bc14a745357805942f587ada8a22ad2ee returned non-zero exit code 1

Auto-merging torch/_inductor/compile_fx.py
Auto-merging torch/_inductor/mkldnn.py
Auto-merging torch/_inductor/pattern_matcher.py
CONFLICT (content): Merge conflict in torch/_inductor/pattern_matcher.py
Auto-merging torch/_meta_registrations.py
error: could not apply ae89899bc14... inductor: rewrite mkldnn fx fusion using pattern_matcher(conv_unary)
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".
Details for Dev Infra team Raised by workflow job

@XiaobingSuper
Copy link
Collaborator Author

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/inductor ciflow/trunk Trigger trunk jobs on your pull request Merged module: cpu CPU specific problem (e.g., perf, algorithm) module: inductor open source release notes: inductor
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

7 participants