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

swolchok · 2022-10-12T21:51:53Z

Stack from ghstack (oldest at bottom):

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

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 · 2022-10-12T21:51:56Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit our office hours

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:

linux-bionic-cuda11.6-py3.10-gcc7 / test (default, 4, 4, linux.4xlarge.nvidia.gpu)

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

`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

c10/test/util/irange_test.cpp

c10/util/irange.h

r-barnes · 2022-10-12T22:21:47Z

@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.

c10/util/irange.h

swolchok · 2022-10-17T22:00:59Z

#85969 is the PR we are blocked on here.

c10/util/irange.h

…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]

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/)

r-barnes · 2022-10-26T20:54:56Z

aten/src/ATen/native/TensorShape.cpp

@@ -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());


Is this a test artefact?

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

swolchok · 2022-10-26T22:09:08Z

@pytorchbot merge -g

pytorchmergebot · 2022-10-26T22:11:03Z

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 · 2022-10-26T22:11:04Z

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 · 2022-10-27T01:04:14Z

@pytorchbot rebase

pytorchmergebot · 2022-10-27T01:06:00Z

@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 · 2022-10-27T01:06:15Z

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 · 2022-10-27T01:10:06Z

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.

…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]

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 · 2022-10-27T03:30:55Z

@pytorchbot merge -g

pytorchmergebot · 2022-10-27T03:34:02Z

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 · 2022-10-27T04:44:29Z

Merge failed

Reason: The following mandatory check(s) failed (Rule superuser):

pull

Dig deeper by viewing the failures on hud

Details for Dev Infra team

Raised by workflow job

swolchok · 2022-10-27T16:13:01Z

@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 · 2022-10-27T16:15:21Z

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]

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 · 2022-10-27T17:06:13Z

@pytorchbot merge -g

pytorchmergebot · 2022-10-27T17:07:43Z

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 · 2022-10-27T19:23:54Z

Merge failed

Reason: The following mandatory check(s) failed (Rule superuser):

pull

Dig deeper by viewing the failures on hud

Details for Dev Infra team

Raised by workflow job

swolchok · 2022-10-31T20:30:24Z

@pytorchbot rebase

pytorchmergebot · 2022-10-31T20:32:33Z

@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 · 2022-10-31T20:32:51Z

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)

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 · 2022-10-31T20:34:07Z

@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]

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 · 2022-11-01T01:49:57Z

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 · 2022-11-02T21:26:43Z

failure definitely looks unrelated

swolchok · 2022-11-02T21:26:50Z

@pytorchbot merge -f

pytorch-bot · 2022-11-02T21:26:53Z

❌ 🤖 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 · 2022-11-02T21:29:40Z

@pytorchbot merge -f 1 failure that sure looks unrelated

pytorch-bot · 2022-11-02T21:29:42Z

❌ 🤖 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 · 2022-11-02T21:32:28Z

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

pytorchmergebot · 2022-11-02T21:34:18Z

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

…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

r-barnes reviewed Oct 12, 2022

View reviewed changes

c10/test/util/irange_test.cpp Outdated Show resolved Hide resolved

c10/test/util/irange_test.cpp Outdated Show resolved Hide resolved

c10/util/irange.h Show resolved Hide resolved

swolchok commented Oct 12, 2022

View reviewed changes

c10/util/irange.h Show resolved Hide resolved

swolchok commented Oct 17, 2022

View reviewed changes

c10/util/irange.h Outdated Show resolved Hide resolved

swolchok requested a review from r-barnes October 17, 2022 22:45

r-barnes approved these changes Oct 26, 2022

View reviewed changes

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Oct 26, 2022

malfet approved these changes Oct 26, 2022

View reviewed changes

swolchok added the topic: not user facing topic category label Oct 27, 2022

pytorchmergebot added the Merged label Nov 2, 2022

pytorchmergebot closed this in 1c0d47c Nov 2, 2022

facebook-github-bot deleted the gh/swolchok/539/head branch June 8, 2023 18:51

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

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

Uh oh!

Conversation

swolchok commented Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

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

❌ 1 Failures

Uh oh!

Uh oh!

Uh oh!

Uh oh!

r-barnes commented Oct 12, 2022

Uh oh!

Uh oh!

swolchok commented Oct 17, 2022

Uh oh!

Uh oh!

r-barnes Oct 26, 2022

Choose a reason for hiding this comment

Uh oh!

swolchok Oct 26, 2022

Choose a reason for hiding this comment

Uh oh!

swolchok commented Oct 26, 2022

Uh oh!

pytorchmergebot commented Oct 26, 2022

Merge started

Uh oh!

pytorchmergebot commented Oct 26, 2022

Merge failed

Uh oh!

swolchok commented Oct 27, 2022

Uh oh!

pytorchmergebot commented Oct 27, 2022

Uh oh!

pytorchmergebot commented Oct 27, 2022

Uh oh!

github-actions bot commented Oct 27, 2022

This PR needs a label

Uh oh!

swolchok commented Oct 27, 2022

Uh oh!

pytorchmergebot commented Oct 27, 2022

Merge started

Uh oh!

pytorchmergebot commented Oct 27, 2022

Merge failed

Uh oh!

swolchok commented Oct 27, 2022

Uh oh!

pytorchmergebot commented Oct 27, 2022

Uh oh!

swolchok commented Oct 27, 2022

Uh oh!

pytorchmergebot commented Oct 27, 2022

Merge started

Uh oh!

pytorchmergebot commented Oct 27, 2022

Merge failed

Uh oh!

swolchok commented Oct 31, 2022

Uh oh!

pytorchmergebot commented Oct 31, 2022

Uh oh!

pytorchmergebot commented Oct 31, 2022

Uh oh!

swolchok commented Oct 31, 2022

Uh oh!

malfet commented Nov 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swolchok commented Nov 2, 2022

Uh oh!

swolchok commented Nov 2, 2022

Uh oh!

swolchok commented Oct 12, 2022 •

edited

Loading

pytorch-bot bot commented Oct 12, 2022 •

edited

Loading

malfet commented Nov 1, 2022 •

edited

Loading