KEMBAR78
Fix inductor `sub` with symbolic integers. by ysiraichi · Pull Request #108518 · pytorch/pytorch · GitHub
Skip to content

Fix inductor sub with symbolic integers. #108518

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 7 commits into from
Closed

Fix inductor sub with symbolic integers. #108518

wants to merge 7 commits into from

Conversation

@ysiraichi
Copy link
Collaborator

@ysiraichi ysiraichi commented Sep 4, 2023 •
edited by pytorch-bot bot
Loading

@pytorch-bot
Copy link

pytorch-bot bot commented Sep 4, 2023 •
edited
Loading

🔗 Helpful Links

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

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

✅ No Failures

As of commit 0dbfb94 with merge base db63bf3 (image):
💚 Looks good so far! There are no failures yet. 💚

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

@ysiraichi
Add comment. on "Fix inductor sub with symbolic integers."
cbd570d 
Fix: #108159

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78 kadeng muchulee8 aakhundov

[ghstack-poisoned]
@ysiraichi
Add support to ExpandView. on "Fix inductor sub with symbolic integ…
fbe751a 
…ers."

Fix: #108159

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78 kadeng muchulee8 aakhundov

[ghstack-poisoned]
@ysiraichi
Fix lint issues. on "Fix inductor sub with symbolic integers."
3b21c33 
Fix: #108159

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78 kadeng muchulee8 aakhundov

[ghstack-poisoned]
peterbell10
peterbell10 reviewed Sep 4, 2023
View reviewed changes
torch/_inductor/lowering.py Outdated
assert alpha is None

# Get the first TensorBox/ExpandView input as the reference tensor for
# sizes, data types, and device.
Copy link
Collaborator

Choose a reason for hiding this comment

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

promote_constants handles this for ir.Constant by wrapping it in an ExpandView:

pytorch/torch/_inductor/lowering.py

Lines 351 to 353 in 51c2e22

ExpandView.create(
ir.Constant(x, ex.get_dtype(), ex.get_device()), list(ex.get_size())
)

I think we could just do the same for IndexingConstant below:

pytorch/torch/_inductor/lowering.py

Line 356 in 51c2e22

out.append(IndexingConstant(x, ex.get_dtype(), ex.get_device()))

@ysiraichi
Update on "Fix inductor sub with symbolic integers."
f16d945 
Fix: #108159

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78 kadeng muchulee8 aakhundov

[ghstack-poisoned]
ysiraichi added a commit that referenced this pull request Sep 4, 2023
@ysiraichi
Fix inductor sub with symbolic integers.
e2a8984 
Fix: #108159

ghstack-source-id: 0a7d24b
Pull Request resolved: #108518
@ysiraichi ysiraichi marked this pull request as draft September 5, 2023 20:42
@ysiraichi
Update on "Fix inductor sub with symbolic integers."
9ed4fec 
Fix: #108159

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78 kadeng muchulee8 aakhundov

[ghstack-poisoned]
ysiraichi added a commit that referenced this pull request Sep 5, 2023
@ysiraichi
Fix inductor sub with symbolic integers.
fb7fa17 
Fix: #108159

ghstack-source-id: 8cdbc02
Pull Request resolved: #108518
peterbell10
peterbell10 reviewed Sep 6, 2023
View reviewed changes
torch/_inductor/lowering.py
)
)
elif isinstance(x, sympy.Expr):
out.append(IndexingConstant(x, ex.get_dtype(), ex.get_device()))
Copy link
Collaborator

Choose a reason for hiding this comment

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

You still need to create an IndexingConstant node here, just wrap it in ExpandView.create.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, I see. Thanks for the help, @peterbell10.

@ysiraichi ysiraichi mentioned this pull request Sep 6, 2023
Closed
ysiraichi added a commit that referenced this pull request Sep 6, 2023
@ysiraichi
Re-land: Break graph on manual_seed.
7c5a6bc 
Trying to re-land #108518.

[ghstack-poisoned]
ysiraichi added a commit that referenced this pull request Sep 6, 2023
@ysiraichi
Re-land: Break graph on manual_seed.
ba988ac 
Trying to re-land #108518.

ghstack-source-id: c8cfe27
Pull Request resolved: #108647
@ysiraichi
Update on "Fix inductor sub with symbolic integers."
0dbfb94 
Fix: #108159

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78 kadeng muchulee8 aakhundov

[ghstack-poisoned]
ysiraichi added a commit that referenced this pull request Sep 6, 2023
@ysiraichi
Fix inductor sub with symbolic integers.
5a49549 
Fix: #108159

ghstack-source-id: 8c7776b
Pull Request resolved: #108518
@ysiraichi ysiraichi marked this pull request as ready for review September 6, 2023 16:28
@ysiraichi
Copy link
Collaborator Author

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Sep 6, 2023
@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: This PR needs a release notes: label
If your changes are user facing and intended to be a part of release notes, please use a label starting with release notes:.

If not, please add the topic: not user facing label.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "topic: not user facing"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

Details for Dev Infra team Raised by workflow job

@ysiraichi
Copy link
Collaborator Author

@pytorchbot merge

1 similar comment
@peterbell10
Copy link
Collaborator

@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

@facebook-github-bot facebook-github-bot deleted the gh/ysiraichi/83/head branch September 10, 2023 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@peterbell10 peterbell10 peterbell10 approved these changes

Assignees

No one assigned

Labels

ciflow/inductor ciflow/trunk Trigger trunk jobs on your pull request Merged module: inductor open source release notes: inductor

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ysiraichi @pytorchmergebot @peterbell10 @pytorchbot