-
Notifications
You must be signed in to change notification settings - Fork 800
[emitter] Ensure dim order and physical tensor memory match #16636
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
base: main
Are you sure you want to change the base?
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/16636
Note: Links to docs will display an error until the docs builds have been completed. ❌ 2 New Failures, 2 Unrelated FailuresAs of commit 31335c6 with merge base 33974d5 ( NEW FAILURES - The following jobs have failed:
FLAKY - The following job failed but was likely due to flakiness present on trunk:
BROKEN TRUNK - The following job failed but was 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. |
This PR needs a
|
76c83f3 to
b787b2a
Compare
|
|
||
| spec.storage = real_tensor.untyped_storage() | ||
|
|
||
| spec.stride = real_tensor.stride() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why didn't we reset stride before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
edit: set it here instead of after .contiguous() call to make sure it's correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh no no i mean before this PR there should be some incontiguous tensor being replaced by contiguous one by calling .contiguous(), and for those tensors they should also update spec. Wondering why we didn't do that before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah not sure. We do update in tensor.py after calling contiguous though:
Lines 189 to 209 in d58c8ee
| def from_tensor(cls, tensor: torch.Tensor, const: bool = False) -> TensorSpec: | |
| if const: | |
| # for non-contigous tensors, convert to a contiguous one | |
| tensor = tensor.contiguous() | |
| # Weights cannot be views during emission or serialization | |
| if tensor.nbytes != tensor.untyped_storage().nbytes(): | |
| tensor = tensor.clone() | |
| spec = cls( | |
| dtype=tensor.dtype, | |
| shape=tensor.shape, | |
| layout=tensor.layout, | |
| const=const, | |
| is_sparse=tensor.is_sparse, | |
| ) | |
| spec.stride = tensor.stride() | |
| spec.dim_order = dim_order_from_stride(spec.stride) | |
| spec.requires_grad = tensor.requires_grad | |
| spec.storage = tensor.untyped_storage() if const else None | |
| return spec |
b787b2a to
6654912
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a bug where channels-last tensors were emitted with mismatched dim_order metadata and physical memory layout. The fix ensures that when processing constant tensors at emission time, both contiguous and channels-last formats are preserved correctly, and the TensorSpec metadata (stride and dim_order) is updated to match the actual tensor's memory layout.
Changes:
- Updated
_emitter.pyto check if tensors are either contiguous or channels_last, and update TensorSpec metadata accordingly - Added
dim_order_from_strideimport to support computing correct dim_order from stride - Added comprehensive test case validating channels-last constant tensor emission with verification of both dim_order metadata and physical storage layout
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| exir/emit/_emitter.py | Updated tensor contiguity check to allow channels-last format and ensure spec metadata is synchronized with actual tensor layout |
| exir/emit/test/test_emit.py | Added test validating channels-last constant tensors are emitted with correct dim_order and NHWC physical storage layout |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6654912 to
31335c6
Compare
GregoryComer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this! It might be worth adding one test for a constant tensor that has neither contiguous nor channels-last dim order. One motivating case was something like torch.randn(2, 16).transpose(1, 0).
Summary
At the emitter, resolved tensors should be in contiguous (NCHW) or channels-last (NHWC) format, with the correct corresponding physical memory.
This PR checks that
Fixes a bug where channels-last tensors were made into NCHW, with the dim order not updated.
Test plan
python -m unittest executorch.exir.emit.test.test_emit.TestEmit.test_emit_channels_last_constant