Skip to content

TensorLayout updates #7870

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
Merged
merged 1 commit into from
Jan 27, 2025
Merged

TensorLayout updates #7870

merged 1 commit into from
Jan 27, 2025

Conversation

@lucylq
Copy link
Contributor

@lucylq lucylq commented Jan 23, 2025

Summary: Addressing comments from D67048723

Differential Revision: D68535453

@pytorch-bot
Copy link

pytorch-bot bot commented Jan 23, 2025 •
edited
Loading

🔗 Helpful Links

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

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

✅ You can merge normally! (1 Unrelated Failure)

As of commit 4df1982 with merge base a836b64 (image):

BROKEN TRUNK - The following job failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

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

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 23, 2025
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D68535453

@lucylq lucylq requested a review from dbort January 23, 2025 02:10
@lucylq lucylq added topic: not user facing module: extension Issues related to code under extension/ and removed topic: not user facing labels Jan 23, 2025
lucylq added a commit to lucylq/executorch-1 that referenced this pull request Jan 23, 2025
@lucylq @facebook-github-bot
TensorLayout updates (pytorch#7870)
5c64044 
Summary:

Addressing comments from D67048723

Differential Revision: D68535453
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D68535453

@lucylq lucylq removed the module: extension Issues related to code under extension/ label Jan 23, 2025
dbort
dbort requested changes Jan 23, 2025
View reviewed changes
Copy link
Contributor

@dbort dbort left a comment

Choose a reason for hiding this comment

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

Thanks for responding to my comments on the previous PR!

runtime/core/tensor_layout.h Outdated
nbytes_(calculate_nbytes(sizes_, scalar_type_)) {}

Span<const uint8_t> dim_order);
TensorLayout(const TensorLayout&) = default;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since all "rule of five" methods are set to default, I thiiink you can avoid mentioning any of them.

If you're depending on the ability to move, assign, and implicit-construct, then please add tests for those cases. If they pass without these default overrides then all the better.

lucylq added a commit to lucylq/executorch-1 that referenced this pull request Jan 24, 2025
@lucylq @facebook-github-bot
TensorLayout updates (pytorch#7870)
11fd4f2 
Summary:

Addressing comments from D67048723

Differential Revision: D68535453
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D68535453

@lucylq lucylq requested a review from dbort January 24, 2025 00:54
lucylq added a commit to lucylq/executorch-1 that referenced this pull request Jan 24, 2025
@lucylq @facebook-github-bot
TensorLayout updates (pytorch#7870)
3b83e46 
Summary:

Addressing comments from D67048723

Differential Revision: D68535453
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D68535453

dbort
dbort approved these changes Jan 24, 2025
View reviewed changes
Copy link
Contributor

@dbort dbort left a comment

Choose a reason for hiding this comment

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

Looking good! Just some final requests for new tests, but the public API looks good to me.

runtime/core/tensor_layout.h
dim_order_(dim_order),
scalar_type_(scalar_type),
nbytes_(calculate_nbytes(sizes_, scalar_type_)) {}
TensorLayout() = delete;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need to do this since you declare a non-default ctor.

https://en.cppreference.com/w/cpp/language/default_constructor

Implicitly-declared default constructor

If there is no user-declared constructor or constructor template for a class type, the compiler will implicitly declare a default constructor as an inline public member of its class.

lucylq added a commit to lucylq/executorch-1 that referenced this pull request Jan 24, 2025
@lucylq @facebook-github-bot
TensorLayout updates (pytorch#7870)
aea31dd 
Summary:

Addressing comments from D67048723

Differential Revision: D68535453
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D68535453

lucylq added a commit to lucylq/executorch-1 that referenced this pull request Jan 24, 2025
@lucylq @facebook-github-bot
TensorLayout updates (pytorch#7870)
8740e8a 
Summary:

Addressing comments from D67048723

Differential Revision: D68535453
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D68535453

@lucylq @facebook-github-bot
TensorLayout updates (pytorch#7870)
4df1982 
Summary:

Addressing comments from D67048723

Reviewed By: JacobSzwejbka

Differential Revision: D68535453
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D68535453

@facebook-github-bot facebook-github-bot merged commit 2170a84 into pytorch:main Jan 27, 2025
46 of 47 checks passed
YIWENX14 pushed a commit that referenced this pull request Jan 28, 2025
@lucylq @YIWENX14
TensorLayout updates
996d941 
Differential Revision: D68535453

Pull Request resolved: #7870
zonglinpeng pushed a commit to zonglinpeng/executorch that referenced this pull request Jan 30, 2025
@lucylq
TensorLayout updates
dfeebee 
Differential Revision: D68535453

Pull Request resolved: pytorch#7870
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dbort dbort dbort approved these changes

Assignees

No one assigned

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported topic: not user facing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@lucylq @facebook-github-bot @dbort