KEMBAR78
[Quant][Inductor] expand quantization conv-binary(-unary) pattern fusion inside inductor by jiayisunx · Pull Request #138051 · pytorch/pytorch · GitHub
Skip to content

[Quant][Inductor] expand quantization conv-binary(-unary) pattern fusion inside inductor #138051

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

[Quant][Inductor] expand quantization conv-binary(-unary) pattern fusion inside inductor #138051

wants to merge 5 commits into from

Conversation

@jiayisunx
Copy link
Collaborator

@jiayisunx jiayisunx commented Oct 16, 2024 •
edited by pytorch-bot bot
Loading

Stack from ghstack (oldest at bottom):

Summary

Expand quantization conv-binary(-unary) pattern fusion inside inductor to support the following two patterns:
Pattern 1:

    Conv(X)   extra input
           \   /
            Add
             |
        Optional(relu)
             |
             Y

Pattern 2:

    extra input   Conv(X)
           \   /
            Add
             |
        Optional(relu)
             |
             Y

cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @ipiszy @yf225 @chenyang78 @kadeng @muchulee8 @ColinPeppler @amjames @desertfire @chauhang @aakhundov

@jiayisunx
Update
485eedc 
[ghstack-poisoned]
@pytorch-bot
Copy link

pytorch-bot bot commented Oct 16, 2024 •
edited
Loading

🔗 Helpful Links

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

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

✅ You can merge normally! (1 Unrelated Failure)

As of commit d1ed386 with merge base e4ad028 (image):

BROKEN TRUNK - The following job failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

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

@jiayisunx
Update
b941fdb 
[ghstack-poisoned]
jiayisunx added a commit that referenced this pull request Oct 16, 2024
@jiayisunx
[Quant][Inductor] expand quantization conv-binary(-unary) pattern fus…
dee848c 
…ion inside inductor

ghstack-source-id: 83394fb
Pull Request resolved: #138051
@jiayisunx jiayisunx added the topic: not user facing topic category label Oct 16, 2024
leslie-fang-intel
leslie-fang-intel requested changes Oct 16, 2024
View reviewed changes
Copy link
Collaborator

@leslie-fang-intel leslie-fang-intel left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Please add the corresponding UT for these patterns.

jiayisunx added a commit that referenced this pull request Oct 21, 2024
@jiayisunx
[Quant][Inductor] expand quantization conv-binary(-unary) pattern fus…
1235985 
…ion inside inductor

ghstack-source-id: 8ef69d8
Pull Request resolved: #138051
@jiayisunx
Copy link
Collaborator Author

extra input

Thanks for your suggestion, I have added the corresponding UT for these patterns.

@jiayisunx
Update
75bba72 
[ghstack-poisoned]
leslie-fang-intel
leslie-fang-intel approved these changes Oct 22, 2024
View reviewed changes
Copy link
Collaborator

@leslie-fang-intel leslie-fang-intel left a comment

Choose a reason for hiding this comment

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

Looks good to me. I think we should further enhance the _qconv2d_add_cpu_test_helper2 by swapping inputs of add.

test/inductor/test_mkldnn_pattern_matcher.py
x1 = self.conv1(x)
tmp = self.add_fn(x1, x2)
if self.use_relu:
tmp = self.relu(tmp)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Further adding a flag to swap inputs of add?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added, thanks for your advice!

jiayisunx added a commit that referenced this pull request Oct 22, 2024
@jiayisunx
[Quant][Inductor] expand quantization conv-binary(-unary) pattern fus…
8b088d7 
…ion inside inductor

ghstack-source-id: 4edb5a9
Pull Request resolved: #138051
@jiayisunx
Update
b8d6e3a 
[ghstack-poisoned]
@jiayisunx jiayisunx requested a review from jansel October 22, 2024 13:29
@jiayisunx jiayisunx added the ciflow/trunk Trigger trunk jobs on your pull request label Oct 23, 2024
jgong5
jgong5 approved these changes Oct 23, 2024
View reviewed changes
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.

small nits on the code comment, others LGTM.

test/inductor/test_mkldnn_pattern_matcher.py Outdated
Comment on lines 946 to 948
X
/ \
Conv(X) extra input
Copy link
Collaborator

Choose a reason for hiding this comment

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

A bit confusing here: the "extra input" doesn't depend on "X", right? Why adding a link here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for your comments! yes, you are right, it's my mistake, I have fixed it.

jiayisunx added a commit that referenced this pull request Oct 23, 2024
@jiayisunx
[Quant][Inductor] expand quantization conv-binary(-unary) pattern fus…
969b401 
…ion inside inductor

ghstack-source-id: 45a06e7
Pull Request resolved: #138051
@jiayisunx
Update
d1ed386 
[ghstack-poisoned]
@jiayisunx
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

SamGinzburg pushed a commit that referenced this pull request Oct 28, 2024
@jiayisunx @SamGinzburg
[Quant][Inductor] expand quantization conv-binary(-unary) pattern fus…
c2aecbe 
…ion inside inductor (#138051)

### Summary
Expand quantization conv-binary(-unary) pattern fusion inside inductor to support the following two patterns:
Pattern 1:
```
    Conv(X)   extra input
           \   /
            Add
             |
        Optional(relu)
             |
             Y
```
Pattern 2:
```
    extra input   Conv(X)
           \   /
            Add
             |
        Optional(relu)
             |
             Y
```

Pull Request resolved: #138051
Approved by: https://github.com/leslie-fang-intel, https://github.com/jansel, https://github.com/jgong5
@github-actions github-actions bot deleted the gh/jiayisunx/33/head branch November 23, 2024 02:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jansel jansel jansel approved these changes

@jgong5 jgong5 jgong5 approved these changes

@leslie-fang-intel leslie-fang-intel leslie-fang-intel approved these changes

Assignees

No one assigned

Labels

ciflow/inductor ciflow/trunk Trigger trunk jobs on your pull request Merged module: inductor open source topic: not user facing topic category

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jiayisunx @pytorchmergebot @jansel @jgong5 @leslie-fang-intel @pytorchbot