Repository navigation
Preserve prepacked-weight alignment across save and reload - #33700
Tianlei Wu (tianleiwu) with Copilot wants to merge 3 commits into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Co-authored-by: tianleiwu <30328909+tianleiwu@users.noreply.github.com>
Tianlei Wu (tianleiwu)
left a comment
There was a problem hiding this comment.
Verdict: COMMENT — the fix is correct and verified end to end (the bug reproduces on main and is gone with this PR). The findings below are non-blocking hardening and maintainability suggestions.
What I verified (Linux x86-64, AMD EPYC 7V12 with AVX2, GCC 13.2, Release; built together with #33701, whose files are disjoint)
- PR head:
MatMulNBits.*:MatMulNBitsLutGemm.*47 passed / 6 skipped (the new end-to-end test ran and passed in all 4 configurations);TensorProtoUtilsTest.*:SessionStateTestSharedInitalizersWithPrePacking.*:SaveWithExternalInitializers.*:ExternalDataLoaderLifetimeTest.*:InferenceSessionTests.CompileApi*56 passed. mainproduction code + this PR's tests:MatMulNBits.SaveAndReloadPrepackedWeightsfails in all 4 configurations ("65 of 65 differ"), and 3TensorProtoUtilsTesttests fail (SetExternalDataInformation,PrepackedExternalDataHasDeterministicAlignedLayout,LoadPrepackedWeightsPreservesAlignment). The new tests do detect the bug.
| # | Severity | Where | Issue |
|---|---|---|---|
| 1 | Suggestion | tensorprotoutils.cc (inline) |
The copy fallback for misaligned mappings is silent and uncommented |
| 2 | Suggestion | tensor_external_data_info.cc (inline) |
Name the MLAS absolute-alignment requirement as the reason for kAllocAlignment |
| 3 | Suggestion (optional) | MatMulNBits<T1>::UseSharedPrePackedBuffers in onnxruntime/contrib_ops/cpu/quantization/matmul_nbits.cc (outside this diff) |
The MLAS packed layout depends on the absolute address, and the failure mode is silently wrong numbers (65/65 outputs wrong in my repro). Consider a cheap check there that the incoming buffer is kAllocAlignment-aligned (return an error otherwise), or a follow-up that makes the packed layout base-relative, so any future producer of misaligned pre-packed buffers fails loudly |
| 4 | Nitpick | Tests | Legacy-file reload is covered only at unit level (offset 1), and the end-to-end test needs an int8 4-bit MLAS kernel (I could only exercise AVX2) |
| 5 | Nitpick | PR description | No validation evidence, and the checklist items are unchecked |
Not run: Windows, macOS, ARM64, AVX-512/VNNI hardware, and the full onnxruntime_test_all / onnxruntime_provider_test suites.
|
Reporter of #33677 here. I validated this PR on AVX-512, which the review above lists as not run. Setup. Intel Xeon Gold 6133 (Skylake-SP; avx512f/bw/dq/cd/vl, no AVX512-VNNI / AVX-VNNI, so MLAS uses its Repro sweep (repro_33677.py
All 9 rows have Unit tests from the same CI artifact, on the same machine (filter
Not covered here: AVX512-VNNI / AVX-VNNI kernels, Windows, macOS and ARM64. |
|
Copilot The RuntimeError: Sections total size for /build/1a/MinSizeRel/libonnxruntime.so of 1607802 exceeds threshold of 1607680 by 122. On-disk size=1586080 Failing job: https://github.com/microsoft/onnxruntime/actions/runs/40786732434/job/123169230991 The overage is only 122 bytes. It comes from the new alignment handling in On this branch, Please update |
Explain MLAS's absolute-address alignment invariant and report the aligned-copy fallback for legacy unaligned blobs, including how re-saving restores memory mapping. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues block the changes preserving prepacked-weight alignment across model save and reload.
No open issues 🎉
What changed in this PR
Preserves prepacked-weight alignment across model save and reload, addressing the AVX2 MatMulNBits correctness issue reported in #33677.
Changes:
- Aligns every saved prepacked blob to 256 bytes, independently of optional page alignment.
- Retains aligned memory mappings and uses aligned copies for unaligned blobs or mapping failures.
- Adds alignment checks and MatMulNBits save/reload regression coverage.
| File | Description |
|---|---|
onnxruntime/test/framework/tensorutils_test.cc |
Verifies saved layout and loaded buffer alignment. |
onnxruntime/test/contrib_ops/matmul_4bits_test.cc |
Checks MatMulNBits output equality after saving and reloading prepacked weights. |
onnxruntime/core/framework/tensorprotoutils.cc |
Enforces load-time alignment with an aligned-copy fallback. |
onnxruntime/core/framework/tensor_external_data_info.cc |
Aligns every prepacked blob before writing it. |
Review effort: Balanced
|
Maintenance completion at The previously reported Android size failure is gone on the current head: AndroidBinarySizeCheckJob_MinimalBaseline succeeds. No test was skipped to hide that failure. Thank you for the additional AVX-512 verification. There are no merge conflicts or unresolved review threads. Normal reviewer/branch-protection approval remains the final merge gate; this maintenance task did not merge the PR. |
Description
Motivation and Context
CPU MatMulNBits with
accuracy_level=4can produce incorrect results on AVX2 after reloading saved prepacked weights. MLAS derives packed-region offsets from absolute pointer alignment, so reloading identical bytes at an unaligned address changes how the buffer is interpreted.session.save_external_prepacked_constant_initializersoutput (1.31.1) #33677