Skip to content
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

[PyTorch] faster empty_tensor_restride via smarter refresh_contiguous #49349

Closed
wants to merge 1 commit into from

Conversation

swolchok
Copy link
Contributor

@swolchok swolchok commented Dec 14, 2020

Stack from ghstack:

empty_tensor_restride knows what memory format it just
restrided (restrode?) to. Don't throw that information away.

Differential Revision: D25539840

NOTE FOR REVIEWERS: This PR has internal Facebook specific changes or comments, please review them on Phabricator!

`empty_tensor_restride` knows what memory format it just
restrided (restrode?) to. Don't throw that information away.

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

**NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D25539840/)!

[ghstack-poisoned]
swolchok added a commit that referenced this pull request Dec 14, 2020
`empty_tensor_restride` knows what memory format it just
restrided (restrode?) to. Don't throw that information away.

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

**NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D25539840/)!

ghstack-source-id: 118540305
Pull Request resolved: #49349
@facebook-github-bot
Copy link
Contributor

💊 CI failures summary and remediations

As of commit ef5c949 (more details on the Dr. CI page):


Commit ef5c949 was recently pushed. Waiting for builds...


This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

This comment has been revised 1 time.

Comment on lines +1586 to +1588
is_channels_last_contiguous_ = false;
is_channels_last_contiguous_ = false;
is_channels_last_3d_contiguous_ = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

is_coniguous_ and is_channels_last_contiguous_ are not mutually exclusive, e.g. contiguous tensor of size (N,C,1,1) has both as true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is there anything useful about this diff, then? I don't understand all the subtleties here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There are useful things, contiguous computations are repeated twice now, and they shouldn't be. But you are right, it's more annoying. I should still do it.

@swolchok
Copy link
Contributor Author

abandoning, not as obvious as I thought

@swolchok swolchok closed this Dec 15, 2020
@facebook-github-bot facebook-github-bot deleted the gh/swolchok/41/head branch January 14, 2021 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants