[MLAS] Add int8 extreme-value coverage for ARM64 SymmQgemm - #31606
Hariharan Seshadri (hariharans29) merged 2 commits into
Conversation
MlasSymmQgemmTest only ever fills A and B via MatrixGuardBuffer's default fill, which is confined to [21, 64) -- even ExecuteLong's large M/N/K sweep never exercises an int8 extreme. Add SymmQgemmS8SignedInputTest, mirroring the QgemmS8U8SignedInputTest pattern from microsoft#29787, with A and B explicitly filled with int8 extremes across a K/M/N/offa grid sized around the kernel's block structure (PackedK=16, StrideM=4, N aligned to 16). Test-only change, nothing under core/mlas/lib/ is touched.
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 Changes recommended
The new extreme-value tests should be skipped or conditionally registered on ARM64 hosts without NEON DotProd support to avoid known failures from the non-dotprod SymmQgemm NEON kernel (#31573).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Adds additional MLAS unit-test coverage for ARM64 symmetric quantized GEMM (SymmQgemm) by introducing a new short-execute test suite that explicitly feeds extreme int8 values into both A and B, ensuring corner cases like INT8_MIN/INT8_MAX are exercised (instead of relying on the default [21,63] fill pattern).
Changes:
- Register a new
SymmQgemmS8SignedInputTestsuite for both single-threaded and threadpool-threaded short-execute runs. - Implement
SymmQgemmS8SignedInputTestfixture that fills A/B with explicit extreme int8 patterns across a small M/N/K/offa grid.
File summaries
| File | Description |
|---|---|
| onnxruntime/test/mlas/unittest/test_symm_qgemm.cpp | Registers the new signed-input short-execute test suite (single-thread + threaded). |
| onnxruntime/test/mlas/unittest/test_symm_qgemm_fixture.h | Adds the new SymmQgemmS8SignedInputTest fixture that fills inputs with extreme int8 values. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
The plain-NEON SymmQgemm kernel has a known int16 accumulator overflow with extreme int8 operands (microsoft#31573); on hosts without FEAT_DotProd the new tests would fail against it. Skip instead, per hariharans29 and Copilot's review feedback.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
onnxruntime/test/mlas/unittest/test_symm_qgemm_fixture.h:136
- The new signed-input tests register under the suite name "SymmQGemm...", but the existing SymmQgemm suites (e.g., MlasSymmQgemmTest::GetTestSuiteName) use "SymmQgemm...". Using a different capitalization makes filtering/triage inconsistent across SymmQgemm tests.
testing::RegisterTest(
Threaded ? "SymmQGemmS8_Int32_SignedInput_Threaded" : "SymmQGemmS8_Int32_SignedInput_SingleThread",
test_name.c_str(),
onnxruntime/test/mlas/unittest/test_symm_qgemm_fixture.h:149
- These tests are expected to skip entirely on ARM64 hosts without FEAT_DotProd; as written, they still register 624 (per mode) individual tests and each one hits GTEST_SKIP. Consider short-circuiting registration when dot-product isn’t available to avoid noisy output and unnecessary per-test overhead on such hosts.
static size_t RegisterShortExecuteTests() {
size_t test_registered = 0;
4c1d32b
into
microsoft:main
Widen each 8-lane int8 product group into the int32 accumulators before multiplying the second half of the packed K block. This avoids the signed int16 overflow that occurs when two `-128 * -128` products share a halfword lane. ### Description * Update the plain-NEON ARM64/AArch64 SymmQgemm S8 kernels to reduce each 8-lane product group into int32 before the next multiply. * Keep the ARM64 and AArch64 assembly implementations in sync. * Remove the temporary non-dotprod test guard introduced in #31606 so the existing signed-input regression coverage runs on the plain-NEON path again. ### Motivation and Context Fixes #31573. The previous `smull` + `smlal` sequence accumulated two int8 products in a signed int16 lane before widening. For the extreme case, `(-128 * -128) + (-128 * -128) = 32768`, which overflows int16. The revised sequence reduces each product group into the int32 accumulators before processing the second half of the packed K block. ### Validation The existing signed-input regression test is re-enabled for non-dotprod ARM64. The repository also contains `onnxruntime_mlas_benchmark` with `SYMMQGEMM/SignedActivation`; no representative non-dotprod Arm64 hardware was available for a trustworthy throughput comparison, so no performance numbers are claimed here.
Summary
The ARM64 symmetric quantized GEMM tests (
MlasSymmQgemmTest) only ever feed A and B withMatrixGuardBuffer::GetBuffer()'s default fill, which is confined to[21, 64)— evenExecuteLong's large M/N/K sweep never touches an int8 extreme. This addsSymmQgemmS8SignedInputTest, mirroring theQgemmS8U8SignedInputTestpattern from #29787, with A and B both explicitly filled with int8 extremes (-128, -1, 0, 1, 127, ...) across a K/M/N/offa grid sized around the kernel's actual block structure (PackedK=16,StrideM=4, N aligned to 16).This is a test-only change; nothing under
onnxruntime/core/mlas/lib/is touched.While adding these tests, I found a correctness bug in the plain-NEON (non-dotprod)
MlasSymQgemmS8KernelNeonkernel and filed it separately as #31573. The new tests here pass because they exercise the SDOT dispatch (default on both machines I verified this on); the NEON dispatch only gets exercised on ARM64 cores without dot-product support.Testing
onnxruntime_mlas_test, full suite, no regressions:*SymmQGemmS8_Int32_SignedInput*: 1248/1248 passed on both (624 cases x SingleThread/Threaded).