Repository navigation
Serialize only a view constant's own bytes in XNNPACK and Vulkan - #24181
Conversation
shoumikhin
added
the
release notes: xnnpack
pytorch-bot
Bot
added
the
module: vulkan
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/24181
Note: Links to docs will display an error until the docs builds have been completed. ⏳ No Failures, 2 PendingAs of commit 5610818 with merge base 5a66091 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
Bot
added
the
CLA Signed
|
@claude review |
|
Claude finished @shoumikhin's task in 3m 39s —— View job The fix is correct, but neither new XNNPACK regression test runs in OSS CI, and the same storage-from-byte-0 read is still in two other serialization paths. Detailed reviewPR Review: #24181SummaryThe PR clones view constants before their bytes are read, so the XNNPACK and Vulkan serializers write only the tensor's own bytes. It also always copies XNNPACK per-channel scales into contiguous storage. Three independent reviewers found no correctness bug in the changed serializer code. The problems are a CI coverage gap for the XNNPACK tests and two untouched paths with the same bug pattern. Testing
Code Quality
RecommendationApprove Nothing in the serializer changes blocks merging. Ideally, move the XNNPACK tests into a file that CI collects before merging, so the fix has regression coverage. I could only read code in this environment: running Python needed approval that wasn't granted. So I didn't run the tests myself, and I'm relying on the PR description that each new test fails without the fix. |
A constant tensor can be a view into a larger tensor, for example a
buffer registered as `packed[8:16]`, or a slice that constant folding
leaves as a view. The XNNPACK and Vulkan serializers copied the bytes
of the whole underlying storage starting at its first byte. For a view
that starts later in the storage, the delegate then got the wrong
weights, and the .pte gave wrong results with no error:
self.register_buffer("weight", torch.randn(3, 8, 8)[1])
F.linear(x, self.weight) # XNNPACK result was far from eager
Both serializers now clone a weight whose storage is larger than the
tensor before reading its bytes. This is the same rule the ExecuTorch
emitter already uses for weights that are views.
XNNPACK per-channel scales had the same problem, and also skipped the
contiguous() step that weights get, so a transposed groupwise scale was
written in the wrong order. Scales are small, so they are now always
copied into contiguous storage of their own before serialization.
The Vulkan path that serializes raw tensors, which only tests use
today, now does the same.
Tested on macOS arm64, each new test failing before this change and
passing after:
- an XNNPACK linear whose weight is a view (output mismatch before)
- an XNNPACK groupwise int4 linear with a transposed scale (output
mismatch before)
- a Vulkan graph builder check of the serialized bytes of a view
constant
- the existing Vulkan serialization test, which now also serializes a
view and a transposed view
The default backends/xnnpack/test suite passes. Run on its own,
test_linear.py passes except test_linear_qd8_{f16,f32}_per_channel_int4,
which fail the same way without this change. A per-channel scale
replaced by an equal view in an exported program was also checked by
hand: the result differed by 4e4 before, and matches after.
6706c96 to
5610818
Compare
|
Thanks for the review. Here is what I did with each point.
|
|
@pytorchbot cherry-pick --onto release/1.6 -c critical |
Cherry picking #24181The cherry pick PR is at #24208 and it is recommended to link a critical cherry pick PR with an issue. Details for Dev Infra teamRaised by workflow job |
What is wrong today
A constant tensor can be a view into a larger tensor. For example:
Constant folding can also leave a weight as a view, like one chunk of the packed
in_proj_weightofnn.MultiheadAttentionin cross-attention.The XNNPACK and Vulkan serializers copy the bytes of the whole underlying storage, starting at its first byte. For a view that starts later in the storage, the delegate gets the wrong weights, and the .pte gives wrong results with no error. In the model above, the largest XNNPACK error was about 6 to 8. For a view that starts at the beginning, the values are right, but the file carries the whole parent.
XNNPACK per-channel scales are read the same way. They also skip the
contiguous()step that weights get, so a transposed groupwise scale is written in the wrong order.What this change does
exir/tensor.py,exir/emit/_emitter.py).What was tested
On macOS arm64. Each of these fails before this change and passes after:
test_fp32_linear_view_weight(XNNPACK): a linear whose weight is a view.test_qd8_linear_transposed_scale(XNNPACK): a groupwise int4 linear with a transposed scale.test_view_constant_serializes_only_its_own_bytes(Vulkan graph builder): checks the bytes of a view constant, no GPU needed.test_serialize_vulkan_binary(existing): now also serializes a view and a transposed view.The default
backends/xnnpack/testrun passes. It skipstest_linear.py, so that file was run directly: all tests pass excepttest_linear_qd8_{f32,f16}_per_channel_int4, which fail the same way without this change. A per-channel scale replaced by an equal view was also checked by hand: off by about 4e4 before, exact after.Not in this change
_tensor_to_bytesinexir/_serialize/_named_data_store.pyhas a similar problem: for a non-contiguous tensor, such as a channels_last slice of a bigger tensor, it writes the whole storage. XNNPACK and Vulkan pass it plain bytes, so this change does not depend on it. It is left for a separate fix.cc @SS-JIA @manuelcandales @digantdesai @cbilgin