Skip to content

Fix: [ATen] add more missing moves - part 2 #89000

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
Skylion007 wants to merge 26 commits into from
Closed

Fix: [ATen] add more missing moves - part 2 #89000

Skylion007 wants to merge 26 commits into from

Conversation

@Skylion007
Copy link
Collaborator

@Skylion007 Skylion007 commented Nov 14, 2022 •
edited
Loading

Applies some more missing std::move found by static analysis. This should improve performance and reduce unnecessary copies. This PR only targets ATen for now.

And before you ask about the edits, std::move is optimal in a ternary operator as copy ellision cannot happen one. The best thing is probably rewriting it as an if else, but ultimately this should be performant enough.
Followup to #88512 and #88514

cc @jerryzh168 @jianyuh @raghuramank100 @jamesr66a @vkuzo @jgong5 @Xia-Weiwen @leslie-fang-intel @VitalyFedyunin @mingfeima @XiaobingSuper @sanchitintel @ashokei @jingxu10

@pytorch-bot
Copy link

pytorch-bot bot commented Nov 14, 2022 •
edited
Loading

🔗 Helpful Links

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

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

❌ 1 Failures

As of commit 6c4ffc5:

The following jobs have failed:

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

@pytorch-bot pytorch-bot bot added the release notes: linalg_frontend release notes category label Nov 14, 2022
@github-actions github-actions bot added module: cpu CPU specific problem (e.g., perf, algorithm) oncall: quantization Quantization support in PyTorch labels Nov 14, 2022
@Skylion007 Skylion007 changed the title [ATen]: add some more missing moves Fix: [ATen] add some more missing moves Nov 14, 2022
@Skylion007 Skylion007 changed the title Fix: [ATen] add some more missing moves Fix: [ATen] add more missing moves - part 2 Nov 14, 2022
@soumith soumith requested a review from swolchok November 15, 2022 00:02
swolchok
swolchok suggested changes Nov 17, 2022
View reviewed changes
Copy link
Contributor

@swolchok swolchok left a comment

Choose a reason for hiding this comment

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

mostly good. i like that you caught a bunch of missing moves of SymInt in particular; CC @ezyang on those since they're easy to introduce. changes requested for unnecessary moves pointed out in comments

@ezyang
Copy link
Contributor

ezyang commented Nov 18, 2022

Thanks! Deferring to swolchok here

@github-actions github-actions bot added the release notes: quantization release notes category label Dec 4, 2022
@Skylion007
Copy link
Collaborator Author

Skylion007 commented Dec 9, 2022

Hmm, @ezyang saw a failure in Torch Dynamo (that only occurs an hour after CI starts oof). Any way to just run the tests that failed?

@ezyang
Copy link
Contributor

ezyang commented Dec 9, 2022

you can rerun failed test from GHA UI. Can you link to the failure, I don't see it

@Skylion007
Copy link
Collaborator Author

Skylion007 commented Dec 9, 2022

https://github.com/pytorch/pytorch/actions/runs/3659725602/jobs/6186425064

@ezyang
Copy link
Contributor

ezyang commented Dec 9, 2022

in GHA ui you can ask to rerun failed tests

@Skylion007
Copy link
Collaborator Author

Skylion007 commented Dec 9, 2022

in GHA ui you can ask to rerun failed tests

I meant for new commits that attempt to fix it, or do you believe it's just a flake?

@ezyang
Copy link
Contributor

ezyang commented Dec 9, 2022

oh sorry, we're just gonna run everything, not your problem lol.

I don't know if it's your problem or not, this PR is so big it's hard to tell. Maybe we should split it in two or something.

@Skylion007
Copy link
Collaborator Author

Skylion007 commented Dec 9, 2022

Ah, it wasn't my fault. Let me revert that change quickly.: b2795d3

@Skylion007
Copy link
Collaborator Author

Skylion007 commented Dec 9, 2022 •
edited
Loading

@ezyang Reverted and merged fixes form master, just kick off the merge process again. Should have realized it wasn't my fault due to it being from the trunk jobs. ;)

@ezyang
Copy link
Contributor

ezyang commented Dec 10, 2022

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Dec 10, 2022

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

pytorchmergebot commented Dec 10, 2022

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

@Skylion007
Copy link
Collaborator Author

Skylion007 commented Dec 10, 2022

Okay, @ezyang merge conflicts dealt with and the only failing test so far is the a flake due to an download error during install.

@Skylion007
Copy link
Collaborator Author

Skylion007 commented Dec 10, 2022

in GHA ui you can ask to rerun failed tests

BTW, it seems I sadly do not have permissions to. :/

@ezyang
Copy link
Contributor

ezyang commented Dec 10, 2022

@pytorchbot merge -f "flaky ci only"

@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Dec 10, 2022

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

This was referenced Dec 10, 2022
Closed
Closed
pytorchmergebot pushed a commit that referenced this pull request Dec 11, 2022
@Skylion007 @pytorchmergebot
Fix perf bug in #90528 (#90630)
184f6b5 
Fixes a minor I noticed in #90528 also a follow up to #89000. @ezyang

Pull Request resolved: #90630
Approved by: https://github.com/ezyang
ezyang added a commit that referenced this pull request Dec 11, 2022
@ezyang
Revert "Fix: [ATen] add more missing moves - part 2 (#89000)"
9e888f1 
This reverts commit 583d216.

[ghstack-poisoned]
ezyang added a commit that referenced this pull request Dec 11, 2022
@ezyang
Revert "Fix: [ATen] add more missing moves - part 2 (#89000)"
8f4e50d 
This reverts commit 583d216.

ghstack-source-id: a64436f
Pull Request resolved: #90642
pytorchmergebot pushed a commit that referenced this pull request Dec 12, 2022
@Skylion007 @pytorchmergebot
[Fix]: remove unnecessary copies in aten, c10, and torch bindings (#9…
7541c9f 
…0629)

Applies various automated fixes that reduces the number of spurious copies in torch, aten, and c10. I also inlined any default dtors that would have made the type trivially destructible.

Follow up to #89000

Pull Request resolved: #90629
Approved by: https://github.com/ezyang
@Skylion007 Skylion007 mentioned this pull request Dec 18, 2022
Closed
pytorchmergebot pushed a commit that referenced this pull request Dec 19, 2022
@Skylion007 @pytorchmergebot
Apply modernize-use-emplace to aten, c10, torch (#91077)
3916d7a 
Apply clang-tidy check modernize-use-emplace. This is slightly more efficient by using an inplace constructor and is the recommended style in parts of the codebase covered by clang-tidy. This just manually applies the check to rest of the codebase. Pinging @ezyang as this is related to my other PRs he reviewed like #89000

Pull Request resolved: #91077
Approved by: https://github.com/ezyang
@JacobSzwejbka JacobSzwejbka mentioned this pull request Dec 20, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ezyang ezyang ezyang approved these changes

@swolchok swolchok Awaiting requested review from swolchok

@lezcano lezcano Awaiting requested review from lezcano

@nikitaved nikitaved Awaiting requested review from nikitaved

@IvanYashchuk IvanYashchuk Awaiting requested review from IvanYashchuk

@jerryzh168 jerryzh168 Awaiting requested review from jerryzh168

@z-a-f z-a-f Awaiting requested review from z-a-f

@salilsdesai salilsdesai Awaiting requested review from salilsdesai

@kimishpatel kimishpatel Awaiting requested review from kimishpatel

@digantdesai digantdesai Awaiting requested review from digantdesai

@jianyuh jianyuh Awaiting requested review from jianyuh

Assignees

No one assigned

Labels

ciflow/trunk Trigger trunk jobs on your pull request Merged module: cpu CPU specific problem (e.g., perf, algorithm) oncall: quantization Quantization support in PyTorch open source release notes: linalg_frontend release notes category release notes: quantization release notes category triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Skylion007 @ezyang @pytorchmergebot @swolchok @bdhirsh @pytorchbot