Skip to content

[PyTorch] Make c10::irange(x) generate the same assembly as for loop #86841

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

Closed
wants to merge 8 commits into from

Conversation

swolchok
Copy link
Contributor

@swolchok swolchok commented Oct 12, 2022

Stack from ghstack (oldest at bottom):

c10::irange(n) generated an extra sar and andn instruction compared to a traditional for loop. now it doesn't.

Differential Revision: D40321009

`c10::irange(n)` generated an extra `sar` and `andn` instruction compared to a traditional `for` loop. now it doesn't.

Differential Revision: [D40321009](https://our.internmc.facebook.com/intern/diff/D40321009/)

[ghstack-poisoned]
@pytorch-bot
Copy link

pytorch-bot bot commented Oct 12, 2022

🔗 Helpful Links

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

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

❌ 1 Failures

As of commit 3ec4c55:

The following jobs have failed:

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

swolchok added a commit that referenced this pull request Oct 12, 2022
`c10::irange(n)` generated an extra `sar` and `andn` instruction compared to a traditional `for` loop. now it doesn't.

Differential Revision: [D40321009](https://our.internmc.facebook.com/intern/diff/D40321009/)

ghstack-source-id: 170212610
Pull Request resolved: #86841
@r-barnes
Copy link
Contributor

@swolchok - Thanks for this. Since if constexpr is part of C++17, we need to hold off on this until PyTorch completes its migration to C++17.

@swolchok
Copy link
Contributor Author

#85969 is the PR we are blocked on here.

…s for loop"

`c10::irange(n)` generated an extra `sar` and `andn` instruction compared to a traditional `for` loop. now it doesn't.

Differential Revision: [D40321009](https://our.internmc.facebook.com/intern/diff/D40321009/)

[ghstack-poisoned]
swolchok added a commit that referenced this pull request Oct 17, 2022
Pull Request resolved: #86841

`c10::irange(n)` generated an extra `sar` and `andn` instruction compared to a traditional `for` loop. now it doesn't.
ghstack-source-id: 170674298

Differential Revision: [D40321009](https://our.internmc.facebook.com/intern/diff/D40321009/)
@swolchok swolchok requested a review from r-barnes October 17, 2022 22:45
@@ -2889,7 +2889,7 @@ Tensor squeeze_qtensor(const Tensor& self, c10::optional<int64_t> dim) {
const auto* per_channel_quantizer = static_cast<at::PerChannelAffineQuantizer*>(quantizer.get());
auto axis = per_channel_quantizer->axis();
int64_t shift = 0;
integer_range<int64_t> dims = dim.has_value() ? integer_range<int64_t>{dim.value(), dim.value() + 1} : c10::irange(self.dim());
integer_range<int64_t> dims = dim.has_value() ? integer_range<int64_t>{dim.value(), dim.value() + 1} : c10::irange(0, self.dim());
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a test artefact?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's necessary for the two forks of the ?: operator to have the same type.

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Oct 26, 2022
@swolchok
Copy link
Contributor Author

@pytorchbot merge -g

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks on your PR pass since you used the green (-g) flag (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: This PR is too stale; the last push date was more than 3 days ago. Please rebase and try again. You can rebase by leaving the following comment on this PR:
@pytorchbot rebase

Details for Dev Infra team Raised by workflow job

@swolchok
Copy link
Contributor Author

@pytorchbot rebase

@pytorchmergebot
Copy link
Collaborator

@pytorchbot successfully started a rebase job. Check the current status here

…s for loop"

`c10::irange(n)` generated an extra `sar` and `andn` instruction compared to a traditional `for` loop. now it doesn't.

Differential Revision: [D40321009](https://our.internmc.facebook.com/intern/diff/D40321009/)

[ghstack-poisoned]
@pytorchmergebot
Copy link
Collaborator

Successfully rebased gh/swolchok/539/orig onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via ghstack checkout https://github.com/pytorch/pytorch/pull/86841)

@github-actions
Copy link
Contributor

This PR needs a 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.

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

@swolchok swolchok added the topic: not user facing topic category label Oct 27, 2022
…s for loop"

`c10::irange(n)` generated an extra `sar` and `andn` instruction compared to a traditional `for` loop. now it doesn't.

Differential Revision: [D40321009](https://our.internmc.facebook.com/intern/diff/D40321009/)

[ghstack-poisoned]
swolchok added a commit that referenced this pull request Oct 27, 2022
Pull Request resolved: #86841

`c10::irange(n)` generated an extra `sar` and `andn` instruction compared to a traditional `for` loop. now it doesn't.
ghstack-source-id: 171748565

Differential Revision: [D40321009](https://our.internmc.facebook.com/intern/diff/D40321009/)
@swolchok
Copy link
Contributor Author

@pytorchbot merge -g

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks on your PR pass since you used the green (-g) flag (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: The following mandatory check(s) failed (Rule superuser):

Dig deeper by viewing the failures on hud

Details for Dev Infra team Raised by workflow job

@swolchok
Copy link
Contributor Author

@pytorchbot rebase

…s for loop"

`c10::irange(n)` generated an extra `sar` and `andn` instruction compared to a traditional `for` loop. now it doesn't.

Differential Revision: [D40321009](https://our.internmc.facebook.com/intern/diff/D40321009/)

[ghstack-poisoned]
@pytorchmergebot
Copy link
Collaborator

Successfully rebased gh/swolchok/539/orig onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via ghstack checkout https://github.com/pytorch/pytorch/pull/86841)

…s for loop"

`c10::irange(n)` generated an extra `sar` and `andn` instruction compared to a traditional `for` loop. now it doesn't.

Differential Revision: [D40321009](https://our.internmc.facebook.com/intern/diff/D40321009/)

[ghstack-poisoned]
swolchok added a commit that referenced this pull request Oct 27, 2022
Pull Request resolved: #86841

`c10::irange(n)` generated an extra `sar` and `andn` instruction compared to a traditional `for` loop. now it doesn't.
ghstack-source-id: 171748565

Differential Revision: [D40321009](https://our.internmc.facebook.com/intern/diff/D40321009/)
@swolchok
Copy link
Contributor Author

@pytorchbot merge -g

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks on your PR pass since you used the green (-g) flag (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: The following mandatory check(s) failed (Rule superuser):

Dig deeper by viewing the failures on hud

Details for Dev Infra team Raised by workflow job

@swolchok
Copy link
Contributor Author

@pytorchbot rebase

@pytorchmergebot
Copy link
Collaborator

@pytorchbot successfully started a rebase job. Check the current status here

…s for loop"

`c10::irange(n)` generated an extra `sar` and `andn` instruction compared to a traditional `for` loop. now it doesn't.

Differential Revision: [D40321009](https://our.internmc.facebook.com/intern/diff/D40321009/)

[ghstack-poisoned]
@pytorchmergebot
Copy link
Collaborator

Successfully rebased gh/swolchok/539/orig onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via ghstack checkout https://github.com/pytorch/pytorch/pull/86841)

pytorchmergebot pushed a commit that referenced this pull request Oct 31, 2022
Pull Request resolved: #86841

`c10::irange(n)` generated an extra `sar` and `andn` instruction compared to a traditional `for` loop. now it doesn't.
ghstack-source-id: 02f4028

Differential Revision: [D40321009](https://our.internmc.facebook.com/intern/diff/D40321009/)
@swolchok
Copy link
Contributor Author

@pytorchbot merge -g

…s for loop"

`c10::irange(n)` generated an extra `sar` and `andn` instruction compared to a traditional `for` loop. now it doesn't.

Differential Revision: [D40321009](https://our.internmc.facebook.com/intern/diff/D40321009/)

[ghstack-poisoned]
swolchok added a commit that referenced this pull request Oct 31, 2022
Pull Request resolved: #86841

`c10::irange(n)` generated an extra `sar` and `andn` instruction compared to a traditional `for` loop. now it doesn't.
ghstack-source-id: 172194311

Differential Revision: [D40321009](https://our.internmc.facebook.com/intern/diff/D40321009/)
@malfet
Copy link
Contributor

malfet commented Nov 1, 2022

If you believe that failures are unrelated to the PR, feel free to merge it with -f flag. Enabling C++17 across all our platforms feels a bit convoluted, but hopefully will get it done soon.

@swolchok
Copy link
Contributor Author

swolchok commented Nov 2, 2022

failure definitely looks unrelated

@swolchok
Copy link
Contributor Author

swolchok commented Nov 2, 2022

@pytorchbot merge -f

@pytorch-bot
Copy link

pytorch-bot bot commented Nov 2, 2022

❌ 🤖 pytorchbot command failed:

@pytorchbot merge: error: argument -f/--force: expected one argument

usage: @pytorchbot merge [-g | -f MESSAGE | -l] [-r [{viable/strict,master}]]

Try @pytorchbot --help for more info.

@swolchok
Copy link
Contributor Author

swolchok commented Nov 2, 2022

@pytorchbot merge -f 1 failure that sure looks unrelated

@pytorch-bot
Copy link

pytorch-bot bot commented Nov 2, 2022

❌ 🤖 pytorchbot command failed:

@pytorchbot: error: unrecognized arguments: failure that sure looks unrelated

usage: @pytorchbot [-h] {merge,revert,rebase,label} ...

Try @pytorchbot --help for more info.

@swolchok
Copy link
Contributor Author

swolchok commented Nov 2, 2022

@pytorchbot merge -f '1 failure that sure looks unrelated'

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes).

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

kulinseth pushed a commit to kulinseth/pytorch that referenced this pull request Nov 5, 2022
…ytorch#86841)

`c10::irange(n)` generated an extra `sar` and `andn` instruction compared to a traditional `for` loop. now it doesn't.

Differential Revision: [D40321009](https://our.internmc.facebook.com/intern/diff/D40321009/)
Pull Request resolved: pytorch#86841
Approved by: https://github.com/r-barnes, https://github.com/malfet
kulinseth pushed a commit to kulinseth/pytorch that referenced this pull request Dec 10, 2022
…ytorch#86841)

`c10::irange(n)` generated an extra `sar` and `andn` instruction compared to a traditional `for` loop. now it doesn't.

Differential Revision: [D40321009](https://our.internmc.facebook.com/intern/diff/D40321009/)
Pull Request resolved: pytorch#86841
Approved by: https://github.com/r-barnes, https://github.com/malfet
@facebook-github-bot facebook-github-bot deleted the gh/swolchok/539/head branch June 8, 2023 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/trunk Trigger trunk jobs on your pull request Merged topic: not user facing topic category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants