Skip to content

[ARM] MLAS: SVE i8mm (svmmla) int8 QGEMM kernels, portable machine code - #31146

Merged
mirounga merged 17 commits into
microsoft:mainfrom
mirounga:rs_sve_qgemm
Aug 21, 2026
Merged

[ARM] MLAS: SVE i8mm (svmmla) int8 QGEMM kernels, portable machine code#31146
mirounga merged 17 commits into
microsoft:mainfrom
mirounga:rs_sve_qgemm

Conversation

@mirounga

Copy link
Copy Markdown
Contributor

Add S8S8 and U8S8 QGEMM compute kernels using the SVE i8mm svmmla instructions, selected at runtime whenever the processor supports SVE with the I8MM extension (HasArmSVE_I8MM). The kernels consume the exact packed A/B panels of the existing NEON smmla/ummla kernels (byte-identical packing code, PackedK=8, same RowSum/ColumnSum zero-point-correction layout), so results are bit-identical to the NEON path; only the inner compute differs (12x8 M-tiles with an 8x12 second shape, svmmla_s32 / svmmla_u32).

The compute kernels ship as portable machine code in the style of Arm's KleidiAI library: every instruction is a raw word (GAS ".inst" / armasm64 "DCD" via aarch64/kai_asm_macros.h, macro set adopted verbatim from KleidiAI), so one generated file assembles under both the GNU assembler and Microsoft armasm64 with no SVE toolchain support required. The SVE intrinsics reference implementation (sve/qgemm_mmla_sve_impl.cpp) remains the regeneration source: sve/gen_sve_asm.py freezes it, verifying the code is fully self-contained (no relocations, no adrp/bl, no literal pools; compiled -fno-stack-protector) and the emitted words are byte-identical to the compiler's object code. The driver/pack/dispatch translation units use no SVE intrinsics and compile with plain AArch64 flags on any compiler, so the dispatch is not OS-gated; Windows runtime detection uses the PF_ARM_SVE_INSTRUCTIONS_AVAILABLE / PF_ARM_SVE_I8MM_INSTRUCTIONS_AVAILABLE feature constants (SDK-#ifdef-guarded), and the sources are wired through the existing cl /P + armasm64 pipeline. cmake option onnxruntime_SVE_QGEMM_ASM (default ON) selects the frozen machine code; OFF builds the intrinsics reference instead.

M == 1 (GEMV-shaped) operations delegate to the NEON mmla dispatch on Linux, where those kernels exist: a single packed row cannot amortize the 2-row mmla pair structure, and the packed-B interchangeability makes the delegation valid for both the unpacked and prepacked paths (measured: M=1 prepacked cells go from up to 1.07x slower to parity).

Measured on Cortex-X925/A725 (SVE VL=128, 5 big + 5 mid cores), full MLAS QGEMM benchmark (76 median cells, all four SignedA/UnsignedA x PackB/ NoPackB variants), pinned to the big cluster, versus the NEON baseline built from the same base commit (real-time medians, lower is better):

Threads:4 (fits the big cluster) 0.84-0.86x (~1.17x speedup)
Threads:16 (oversubscribed on 5c) 0.93-0.97x
Threads:1, M=1 rows 0.98-1.02x (parity, after the
M==1 delegation)
All 76 cells 0.932x

The frozen machine code measures at parity with the intrinsics build overall (0.932x vs 0.931x); the Threads:4 band pays ~1% for the kernel now being an out-of-line call rather than force-inlined into the operation loop.

Correctness: full onnxruntime_mlas_test with the SVE dispatches active by default: 33213 passed, 0 failed. Results are bit-identical to the NEON kernels (exact integer accumulation over identical packed operands).

Add S8S8 and U8S8 QGEMM compute kernels using the SVE i8mm svmmla
instructions, selected at runtime whenever the processor supports SVE with
the I8MM extension (HasArmSVE_I8MM). The kernels consume the exact packed
A/B panels of the existing NEON smmla/ummla kernels (byte-identical packing
code, PackedK=8, same RowSum/ColumnSum zero-point-correction layout), so
results are bit-identical to the NEON path; only the inner compute differs
(12x8 M-tiles with an 8x12 second shape, svmmla_s32 / svmmla_u32).

The compute kernels ship as portable machine code in the style of Arm's
KleidiAI library: every instruction is a raw word (GAS ".inst" / armasm64
"DCD" via aarch64/kai_asm_macros.h, macro set adopted verbatim from
KleidiAI), so one generated file assembles under both the GNU assembler and
Microsoft armasm64 with no SVE toolchain support required. The SVE
intrinsics reference implementation (sve/qgemm_mmla_sve_impl.cpp) remains
the regeneration source: sve/gen_sve_asm.py freezes it, verifying the code
is fully self-contained (no relocations, no adrp/bl, no literal pools;
compiled -fno-stack-protector) and the emitted words are byte-identical to
the compiler's object code. The driver/pack/dispatch translation units use
no SVE intrinsics and compile with plain AArch64 flags on any compiler, so
the dispatch is not OS-gated; Windows runtime detection uses the
PF_ARM_SVE_INSTRUCTIONS_AVAILABLE / PF_ARM_SVE_I8MM_INSTRUCTIONS_AVAILABLE
feature constants (SDK-#ifdef-guarded), and the sources are wired through
the existing cl /P + armasm64 pipeline. cmake option
onnxruntime_SVE_QGEMM_ASM (default ON) selects the frozen machine code;
OFF builds the intrinsics reference instead.

M == 1 (GEMV-shaped) operations delegate to the NEON mmla dispatch on
Linux, where those kernels exist: a single packed row cannot amortize the
2-row mmla pair structure, and the packed-B interchangeability makes the
delegation valid for both the unpacked and prepacked paths (measured: M=1
prepacked cells go from up to 1.07x slower to parity).

Measured on Cortex-X925/A725 (SVE VL=128, 5 big + 5 mid cores), full MLAS
QGEMM benchmark (76 median cells, all four SignedA/UnsignedA x PackB/
NoPackB variants), pinned to the big cluster, versus the NEON baseline
built from the same base commit (real-time medians, lower is better):

  Threads:4 (fits the big cluster)    0.84-0.86x  (~1.17x speedup)
  Threads:16 (oversubscribed on 5c)   0.93-0.97x
  Threads:1, M=1 rows                 0.98-1.02x  (parity, after the
                                                   M==1 delegation)
  All 76 cells                        0.932x

The frozen machine code measures at parity with the intrinsics build
overall (0.932x vs 0.931x); the Threads:4 band pays ~1% for the kernel now
being an out-of-line call rather than force-inlined into the operation
loop.

Correctness: full onnxruntime_mlas_test with the SVE dispatches active by
default: 33213 passed, 0 failed. Results are bit-identical to the NEON
kernels (exact integer accumulation over identical packed operands).
@mirounga
mirounga marked this pull request as draft July 29, 2026 21:54
Comment thread onnxruntime/core/mlas/lib/sve/gen_sve_asm.py Fixed
Comment thread onnxruntime/core/mlas/lib/sve/gen_sve_asm.py Fixed
Comment thread onnxruntime/core/mlas/lib/sve/gen_sve_asm.py Fixed
The svmmla kernel was correct at any vector length but scaled at none. Every
A and B load was gated by svptrue_pat_b8(SV_VL16), so segments 1..N of each
operand were zero-filled and each svmmla did one 128-bit segment of work no
matter how wide the vector was. At VL=128 that costs nothing, because SVE and
NEON are the same width there - which is why it went unnoticed: all previous
benchmarking of this kernel was done on VL=128 hardware.

On a 256-bit machine it is not merely a missed opportunity but a significant
regression. Measured on Graviton3 (Neoverse V1, VL=256) against the NEON
smmla/ummla kernels this path replaces, the old kernel was 1.308x slower
overall and 1.580x slower with all cores busy: two 256-bit SVE pipes doing
128 bits of useful work each, against NEON's four 128-bit pipes at full width.

Rework the compute so the work per instruction follows the vector length:

- Load A with svld1rq_s8 (LD1RQB), which replicates one packed row-pair quad
  to every 128-bit segment, and load B full width. The packed layout already
  stores consecutive column pairs 16 bytes apart, so a wider load places them
  in consecutive segments. One svmmla then covers two rows by two columns per
  segment - 2 columns at VL=128, 4 at VL=256, 8 at VL=512 - from an unchanged
  instruction count.
- The uzp1/uzp2 output stage needs no change: over 64-bit granules it gathers
  each row's halves across all segments, so it simply yields proportionally
  more contiguous output columns as the vector grows.
- Build the accumulator seed with vector code instead of scalar. zip1 over
  64-bit granules expands ColumnSum and ZeroPointB into the per-segment
  [ca,cb,ca,cb] pattern, and the RowSum [r0,r0,r1,r1] pattern is one hoisted
  ld1rqw, replacing four scalar multiplies and a stack round-trip per
  accumulator.
- Operand loads are capped at 64 bytes, the size of a k-block, because the
  final panel of a k-slice ends exactly at the end of the packed buffer. The
  kernel therefore scales fully through VL=512 and remains correct beyond it.

The packed layouts are deliberately unchanged. ORT can persist prepacked
weights and memory-map them back, so a buffer packed on one machine may be
loaded on another; a vector-length dependent layout would silently produce
wrong results across machines. Keeping the layout also preserves bit-exactness
with the NEON kernels, the M == 1 delegation to NEON, and the public PackB
size and API. Everything used here is base SVE, since Neoverse V1 has no SVE2
and so no quad-word uzp forms.

Two defects found while reworking this:

- Accumulators whose columns fall entirely past CountN still issued a B load,
  which at VL=256 can address a panel that was never allocated. Those loads
  now clamp to the last valid aligned column; their results were already
  discarded by the store mask.
- onnxruntime_SVE_QGEMM_ASM was a plain set() rather than a cache option(),
  so -Donnxruntime_SVE_QGEMM_ASM=OFF was ignored and the intrinsics reference
  - the regeneration source for the checked-in assembly - could not be built.
  This mirrors the same defect fixed for the elementwise kernels.

The frozen machine code is regenerated from the new kernel and verified
byte-identical to the compiler's object after reassembly. sve/gen_sve_asm.py
is updated to the version already used for the elementwise kernels, whose
section-driven parser tolerates the local labels gcc emits inside a function;
the two copies are now identical.

Measured, all against the NEON kernels built from the same base commit
(real-time medians, lower is better):

  Graviton3, Neoverse V1, VL=256, 76 QGEMM cells
    all cores        1.580x  ->  0.848x
    oversubscribed   1.315x  ->  0.965x
    all cells        1.308x  ->  0.944x    (1.39x faster than the old kernel)
  cells slower than NEON by >2%:  56/76  ->  17/76

  Cortex-X925, VL=128, same 76 cells
    all cells        0.943x  ->  0.957x    (~1.5% given up)

Doubling the vector accounts for about 16% of the Graviton3 gain at four
threads, measured by forcing VL=128 on the same binary with prctl; the rest
comes from the restructuring itself. The kernel remains faster than NEON at
VL=128, and the small loss there is the price of a single kernel that behaves
well at every vector length.

Correctness: full onnxruntime_mlas_test on the shipping frozen-assembly build,
33213 passed and 0 failed; on Graviton3, the QGemm suite passes both at native
VL=256 and with the vector length forced to 128 on the same binary, which
confirms the kernel reconfigures at run time in both directions. Integer
accumulation is exact, so these are bit-exactness results rather than
tolerance checks.
MlasGemmQuantCopyPackA for the smmla/ummla kernel types reduced its 4-lane
RowSums accumulator with a vextq + vaddq pair followed by a brace-initialised
int32x2_t/uint32x2_t. MSVC cannot brace-initialise the __n64 vector types, so
these translation units are GCC/clang-only -- which is one of the reasons the
NEON mmla kernels are Linux-gated.

Replace the three-instruction sequence with the single pairwise-add it
computes:

    ext = vextq(p, p, 1)          -> {p1, p2, p3, p0}
    add = vaddq(p, ext)           -> {p0+p1, p1+p2, p2+p3, p3+p0}
    {add[0], add[2]}              -> {p0+p1, p2+p3}
                                  == vpadd(vget_low(p), vget_high(p))

Bit-identical, portable, and three instructions become one. Applied to all 25
sites in each packer. (The SVE copies of these packers carry the same change,
in the following commit, so they stay textually identical to the NEON
originals they were derived from.)

No functional change: integer math is exact and the packed layout is
unchanged, so results are bit-for-bit identical.
…at -O2

Three related changes to the SVE i8mm QGEMM kernel and the tooling that
freezes it to portable machine code.

1. x18 ABI violation (correctness, and a hard blocker off Linux)

   The frozen kernel used x18 in 116 instructions. x18 is the AArch64
   platform register: Windows ARM64 reserves it for the TEB, Darwin reserves
   it, and Linux shadow-call-stack builds use it. Frozen bytes ship on every
   platform, so this corrupted the TEB on Windows -- observed as an access
   violation writing 0x250 from inside ntdll on the first QGemm test. A
   save/restore wrapper is not a fix; the OS may rewrite x18 at any context
   switch.

   gen_sve_asm.py now compiles with -ffixed-x18 by default and hard-fails if
   any x18/w18 reference survives into a frozen section, alongside the
   existing relocation/adrp/bl/literal-pool checks -- it previously had no
   reserved-register check at all. cmake gains ORT_SVE_ABI_FLAGS, defined
   once and applied to every SVE translation unit, so the intrinsics and
   frozen builds are produced from identically-compiled code.

   Measured cost of -ffixed-x18 on X925: +10.4% instructions per kernel and
   ~2-4% runtime. Correctness beats it.

2. Dedicated M=1 path, replacing the NEON delegation

   At M == 1 only one of mmla's two A rows is real, so 32 MACs/instruction
   degrade to 16 -- exactly a dot-product kernel's rate. Parity is therefore
   the ceiling, and the previous 1.26x deficit was per-call overhead, not the
   unpaired row. The single-row path now seeds RowSum with svdup instead of
   building a 4-int array on the stack and reloading it, loads the A quad
   with one svdup instead of memcpy + 2 dup + zip (this sits in the K loop),
   and stores with uzp1 only rather than computing uzp2 and discarding it.
   All three are safe because the mmla's row-1 lanes are never stored.

   M=1 PackB: 1.30x -> 1.02x versus NEON. That removes the reason for the
   __linux__-guarded delegation to the NEON mmla dispatch, so it is deleted
   along with its OS conditional.

3. Freeze at -O2

   gen_sve_asm.py defaulted to -O3 while every consumer builds -O2. On the
   shipping frozen path -O3 measured 7-8% slower at M=1 and flat at M>1, so
   the default now matches cmake.

Also carries the portable-RowSums change from the previous commit into the
SVE copies of the packers, keeping them identical to the NEON originals.

Full onnxruntime_mlas_test: 33213 passed, 0 failed. Integer QGEMM math is
exact, so that is a bit-exactness proof against the NEON kernels.
bench_qgemm.cpp hardcoded `constexpr bool b_is_signed = true`, so every
capture -- including the four named "UnsignedA" -- measured U8S8. The U8U8
dispatch, which is a distinct kernel selection, had no benchmark coverage at
all.

Make b_is_signed a parameter (it was already threaded through
MlasGemmPackBSize, MlasGemmPackB and GemmShape.BIsSigned) and add
UnsignedABPackB / UnsignedABNoPackB.
MLAS_GEMM_U8X8_KERNEL_UMMLA_SVE was already a complete U8X8 kernel type:
MlasGemmQuantFixupZeroPointB and CopyPackB both key off BIsSigned, and
unsigned B is the simpler case -- no 0x80 bit-flip and no zero-point fixup,
and svmmla_u32 wants unsigned operands either way. The NEON ummla dispatch
already serves both U8U8 and U8S8 from this kernel type. Only the dispatch
assignment was missing.

U8U8 at M>1 versus the NEON kernel: 1.02x -> 0.83x prepacked (17-19% faster),
0.98x -> 0.88x unpacked. U8S8 and S8S8 are unchanged within noise, and
*QGemm* stays at 9308/0 -- the U8U8 tests cover both the packed and unpacked
paths, so that is a bit-exactness proof for the newly routed kernel.
Three build-time knobs existed only to A/B the dedicated single-row path
against its predecessor from one source tree, and should not ship:

  * MLAS_SVE_QGEMM_M1_LEAN -- selected between the new M == 1 block and the
    old shared tail path. With it at the default 1 the tail path's Rows == 1
    branch was unreachable, which in turn kept MlasQGemmSveLoadASingleRow
    (the memcpy + 2 dup + zip loader the new path replaced) alive as dead
    code. Both are removed; the M == 1 block is now unconditional and the
    tail path handles Rows in {4, 2} only.

  * MLAS_SVE_QGEMM_M1_COLGROUPS -- pinned to 4 by a static_assert, since the
    body declares exactly four accumulators and SVE's sizeless types cannot
    live in an array. It advertised tunability that did not exist; the count
    is now a literal with the constraint explained in a comment.

No functional change: the frozen kernel regenerated from the cleaned source
is byte-identical to the previous one, so this is source hygiene with zero
codegen impact.
onnxruntime_USE_SVE was force-disabled on anything but Linux/aarch64, so the
Windows wiring already present in onnxruntime_mlas.cmake -- the QGEMM driver
TUs plus the frozen machine-code kernels assembled by armasm64 via the cl.exe
preprocessing step -- was unreachable dead configuration. Nothing could build
it, which is also why the x18 ABI violation in the frozen kernel went
unnoticed until someone tried.

Add an MSVC/ARM64 arm to the gate. No SVE toolchain support is required
there: only the QGEMM path is built, its compute kernels come from
aarch64/qgemm_mmla_sve_asm.S as raw instruction words, and the driver/pack
translation units are plain C++. ORT_SVE_ABI_FLAGS is a GCC flag and applies
only to the Linux arm, which compiles the SVE intrinsics sources.

Two things in platform.cpp are GCC-only and must stay off Windows:

  * the sve/mlasi_sve.h include (#pragma GCC target, SVE intrinsics);
  * the SVE elementwise dispatch, whose kernels are intrinsics translation
    units built only on the Linux arm -- the routines keep their existing
    defaults on Windows.

Both are gated on !defined(_WIN32), matching the existing guard on the FP16
block. NOTE: the portable elementwise rework (rs_sve_eltwise) makes those
kernels buildable everywhere; that work should delete this guard rather than
inherit it.

Linux is unaffected -- it still takes the existing LINUX/aarch64 arm.
Brings in 43 commits, including the portable SVE elementwise kernels and the
Windows ARM64 SVE enablement that landed upstream. Three conflicts, all
resolved as unions rather than picking a side:

  * sve/gen_sve_asm.py (add/add) -- both branches grew a generator. Kept this
    branch's, which is a strict superset: it adds the x18 reserved-register
    check, -ffixed-x18 by default, and --cflag. The only line unique to main
    was the -O3 default, deliberately changed here to -O2 to match how cmake
    actually builds these sources (-O3 measured 7-8% slower at M == 1).
    Note this makes the x18 check govern elementwise regeneration too, where
    the frozen output was previously x18-free by luck rather than by
    construction.

  * core/common/cpuid_info.cc -- took main's non-destructive
    `has_arm_sve_ = has_arm_sve_ || ...` (it preserves the cpuinfo result
    instead of overwriting it) and kept this branch's SVE-i8mm detection,
    which main lacks and which gates the svmmla QGEMM dispatch.

  * cmake/onnxruntime_mlas.cmake -- the Windows block now builds both the
    elementwise kernels (from main) and the QGEMM kernels (from here); the
    Linux block keeps main's onnxruntime_SVE_ELEMENTWISE_ASM structure
    alongside onnxruntime_SVE_QGEMM_ASM, with ORT_SVE_ABI_FLAGS applied to
    both intrinsics paths.

The preceding commit reverted this branch's own Windows SVE enablement: main
already had it, in a better form (WIN32 rather than MSVC, and a portable
mlasi_sve.h that needs no _WIN32 guard on the elementwise dispatch).

Verified on X925: onnxruntime_mlas_test 37734 passed, 0 failed;
*QGemm* 12472 passed, 0 failed.
Comment thread onnxruntime/core/mlas/lib/sve/gen_sve_asm.py Fixed
@mirounga
mirounga marked this pull request as ready for review August 13, 2026 01:43
@mirounga
mirounga requested review from Hariharan Seshadri (hariharans29) and a lite review from Copilot August 13, 2026 01:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 adds ARM64 MLAS int8 QGEMM compute kernels that use SVE i8mm (svmmla) and are selected at runtime when HasArmSVE_I8MM is available, with an option to ship the compute core as portable frozen machine code (KleidiAI-style) or as an SVE-intrinsics reference implementation.

Changes:

  • Add SVE i8mm QGEMM kernel interfaces, shared compute core, and S8S8/U8X8 driver/packing/dispatch translation units.
  • Extend runtime capability detection/dispatch to prefer SVE i8mm kernels (and cover U8U8 via the existing U8X8 kernel type).
  • Enhance the SVE asm-freezing generator to enforce reserved-register constraints (notably x18) and adjust default optimization/settings.

Reviewed changes

Copilot reviewed 14 out of 15 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
onnxruntime/test/mlas/bench/bench_qgemm.cpp Adds missing benchmark coverage for the U8U8 dispatch path.
onnxruntime/core/mlas/lib/sve/qgemm_mmla_sve.h Declares extern “C” compute-kernel entrypoints shared by asm and intrinsics builds.
onnxruntime/core/mlas/lib/sve/qgemm_mmla_sve_impl.cpp Adds SVE-intrinsics reference compute implementation used for regeneration and optional builds.
onnxruntime/core/mlas/lib/sve/qgemm_mmla_kernel_sve.h Introduces the shared SVE i8mm compute core and tiling/store logic.
onnxruntime/core/mlas/lib/sve/qgemm_kernel_smmla_sve.cpp Adds S8S8 driver/packing TU that calls into the SVE compute entrypoint.
onnxruntime/core/mlas/lib/sve/qgemm_kernel_ummla_sve.cpp Adds U8X8 (U8S8/U8U8) driver/packing TU that calls into the SVE compute entrypoint.
onnxruntime/core/mlas/lib/sve/gen_sve_asm.py Strengthens frozen-code validation (reserved register checks) and adds flag plumbing.
onnxruntime/core/mlas/lib/qgemm_kernel_smmla.cpp Refactors row/column sum reductions using vpadd for the NEON i8mm path.
onnxruntime/core/mlas/lib/qgemm_kernel_ummla.cpp Refactors row/column sum reductions using vpadd for the NEON i8mm path.
onnxruntime/core/mlas/lib/platform.cpp Prefers SVE i8mm QGEMM dispatch when available; clarifies Linux-only NEON i8mm asm notes.
onnxruntime/core/mlas/lib/mlasi.h Declares new SVE QGEMM dispatch entrypoints under MLAS_USE_SVE.
onnxruntime/core/common/cpuid_info.cc Adds Windows SVE i8mm feature detection via PF_ARM_SVE_I8MM_INSTRUCTIONS_AVAILABLE when present.
cmake/onnxruntime_mlas.cmake Wires new SVE QGEMM sources and adds onnxruntime_SVE_QGEMM_ASM option.
cmake/CMakeLists.txt Introduces shared SVE ABI compile flags (-ffixed-x18) for SVE translation units.
Suppressed comments (1)

onnxruntime/core/mlas/lib/sve/qgemm_kernel_smmla_sve.cpp:35

  • The NOTE block claims loads are predicated to 16 bytes and that only the first 128-bit segment is used on wider-VL SVE. The actual SVE core (qgemm_mmla_kernel_sve.h) derives Segments from svcntb() and intentionally scales across multiple 128-bit segments (capped at a 64-byte k-block), so this NOTE looks stale/misleading.
    NOTE (v1): the packed layout is the 128-bit-oriented NEON layout, so each
    svmmla consumes a single 128-bit segment. On this hardware SVE VL == 128
    bits, giving throughput parity with NEON smmla. Loads are predicated to 16
    bytes so the kernel remains correct (using only the first 128-bit segment)
    on wider-VL implementations; a fully VL-agnostic packing is a future

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread onnxruntime/core/mlas/lib/sve/qgemm_mmla_kernel_sve.h
Comment thread onnxruntime/core/mlas/lib/sve/qgemm_mmla_kernel_sve.h
Comment thread onnxruntime/core/mlas/lib/sve/qgemm_kernel_smmla_sve.cpp Outdated
Comment thread onnxruntime/core/mlas/lib/sve/qgemm_kernel_ummla_sve.cpp Outdated
Comment thread onnxruntime/core/mlas/lib/sve/qgemm_kernel_smmla_sve.cpp Outdated
Comment thread onnxruntime/core/mlas/lib/sve/qgemm_kernel_ummla_sve.cpp Outdated
@hariharans29

Copy link
Copy Markdown
Member

Review — PR #31146: [ARM] MLAS: SVE i8mm (svmmla) int8 QGEMM kernels, portable machine code

Scope

Large but well-structured PR. Adds SVE i8mm (svmmla_s32 / svmmla_u32) inner kernels for S8S8 / U8S8 / U8U8 QGEMM. New physical structure:

  • Shared vector-length-agnostic compute core: qgemm_mmla_kernel_sve.h — one templated routine (MlasQGemmMmlaKernelSve<AUnsigned>) parameterised over signed/unsigned A.
  • Two interchangeable frozen builds of that core:
    1. Intrinsics reference: qgemm_mmla_sve_impl.cpp — requires a -march=...+sve+i8mm compiler.
    2. KleidiAI-style raw-word assembly: aarch64/qgemm_mmla_sve_asm.S (~5161 lines of KAI_ASM_INST(0x...) words) — assembles under both GAS .inst and MSVC armasm64 DCD with no SVE toolchain.
  • Generator: sve/gen_sve_asm.py freezes (1) → (2) and enforces the position-independence + ABI invariants.
  • Driver/pack/dispatch TUs (qgemm_kernel_smmla_sve.cpp, qgemm_kernel_ummla_sve.cpp) are plain C++ (no SVE intrinsics), so they build with normal AArch64 flags on any platform.
  • CMake option onnxruntime_SVE_QGEMM_ASM (default ON) selects which of the two compute builds is linked.
  • Runtime selection: platform.cpp installs the SVE dispatches when MLAS_CPUIDINFO::HasArmSVE_I8MM() returns true. Author cited: measured ~1.17x speedup at Threads:4 on Cortex-X925 vs the NEON smmla baseline, 33213 MLAS tests passing, bit-identical to NEON.

The x18 ABI fix — most important non-perf change here

Commit 3e2c9843 ("SVE QGEMM: fix x18 ABI violation, add a lean M=1 path, freeze ..."). The problem the author caught: frozen bytes ship on every platform, but x18 is the platform-reserved register on Windows ARM64 (TEB pointer), Darwin (reserved), and Linux shadow-call-stack builds. A single kernel that allocates x18 corrupts the TEB the moment those bytes execute on Windows (observed: 0xC0000005 writing 0x250 inside ntdll).

Fix has three layers, and they need each other:

  1. Compiler-side: -ffixed-x18 in ORT_SVE_ABI_FLAGS, applied both to the SVE intrinsics reference TU and to the non-frozen build path (the ASM=OFF selection). The header comment in cmake/CMakeLists.txt calls this out clearly: applying to only one path would produce different bytes between ASM=ON and ASM=OFF.
  2. Generator-side: RESERVED_REGS = ("x18",) + RESERVED_REG_RE in gen_sve_asm.py; freezing hard-aborts if w18/x18 survives into a frozen section. Belt-and-braces if a future compiler ignores -ffixed-x18 or a mis-inlined helper leaks the register. This is exactly the right place to gate.
  3. Documentation-side: The exact same rationale appears verbatim in both the CMake comment and the generator docstring, and both cross-reference each other. Reviewer-friendly. ✓

Compiler-flag-only was not a fix — save/restore around the kernel isn't a fix either since the OS may rewrite x18 at any context switch. The tri-layer approach is correct.

Nit: --no-default-cflags was added to the generator ("For testing the reserved-register check only — a real regeneration without them will abort."). Good — gives a way to test the audit without letting it slip into a real regen.

Vector-length agnostic scaling — mathematical bound

The kernel is genuinely VL-agnostic and the bound is tight. Key mechanism (qgemm_mmla_kernel_sve.h MlasQGemmSveSegments):

static MLAS_FORCEINLINE size_t MlasQGemmSveSegments(void)
{
    const size_t VectorBytes = svcntb();
    return (VectorBytes < 64 ? VectorBytes : 64) / 16;
}

So Segments = min(VL, 64B) / 16B. This is capped at the 64-byte k-block boundary because the packed layout is [k-block][col-pair][2×8] and one B load must not span into the next k-block. VL scaling:

VL Segments ColsPerAcc ColsPerPass (Rows=8, 6 groups)
128 1 2 12
256 2 4 24
512 4 8 48
>512 4 (capped) 8 (capped) 48

Author states: verified bit-exact on Neoverse V1 at native VL=256 and prctl-forced VL=128. ✓ The cap at VL=512 is documented — beyond that would need cross-segment reduction. Fair engineering trade.

MlasQGemmSveSafeCol — UB avoidance for tail columns

Subtle correctness check. A pass can span past CountN, so the naïve ColumnSumBuffer + col would be one-past-one-past-the-end pointer arithmetic (UB). The MlasQGemmSveSafeCol helper snaps the column to (((CountN - 1) / ColsPerAcc) * ColsPerAcc) when out of range:

return (col < CountN) ? col : (((CountN - 1) / ColsPerAcc) * ColsPerAcc);

Then MlasQGemmSveSeed guards the load with an all-false predicate (svwhilelt_b32_u64(0, 0)) when ColsValid == 0, and the accumulator is never stored (both MlasQGemmSveStoreSingleRow and MlasQGemmSveStoreAccPair early-return on ColsValid == 0).

Two things I want to flag explicitly:

  • The pointer must still be in bounds even though the load reads nothing — the substitute column is always a multiple of ColsPerAcc, so it's guaranteed to be within [0, CountN). ✓
  • Author's commit a22435b ("SVE QGEMM: avoid out-of-bounds pointer arithmetic in the seed") is the fix here. Landed correctly.

Follow-up spot to watch in future refactors: if anyone changes the store helpers to unconditionally read d0/d1 before the ColsValid == 0 early-return, the substitute-column trick silently starts writing to memory that isn't the caller's. Both MlasQGemmSveStoreSingleRow and MlasQGemmSveStoreAccPair start with the guard — do not move the pg computation above the if (ColsValid == 0) return;.

The M=1 lean path

Author's commit e99206c dropped the "delegate to NEON at M=1" scaffolding after adding a dedicated single-row path in the shared core. Rationale in code:

mmla cannot beat udot at M == 1: only one of the two A rows is real, so 32 MACs/instruction degrade to 16 — exactly udot's rate. Parity is therefore the ceiling here, and the entire job of this path is to strip the per-call overhead...

Three specific savings called out in the header comment: (a) svdup_n_s32(RowSumBuffer[0]) replaces the 4-int stack array + ld1rq, (b) MlasQGemmSveLoadASingleRowDup uses one dup instead of memcpy + 2 dup + zip, (c) MlasQGemmSveStoreSingleRow skips uzp2 entirely. Author's numbers: "M=1 prepacked cells go from up to 1.07x slower to parity." Consistent with the reasoning. ✓

Frozen-file review pragmatics

I did not line-by-line audit the 5161-line qgemm_mmla_sve_asm.S. Instead:

  • The freeze pipeline enforces (via gen_sve_asm.py) a positive verification: object emitted from the intrinsics reference is byte-identical to the emitted hex words; FORBIDDEN_MNEMONIC_RE blocks adrp/adr/bl/blr; LITERAL_LOAD_RE blocks literal-pool loads; RESERVED_REG_RE blocks x18. So position-independence + ABI-safety are provable properties of any file that comes out of the generator.
  • The path forward for future correctness: any change must be made to the intrinsics reference and re-frozen. The .S file must never be hand-edited. Header comment on the .S already states this ("GENERATED FILE - DO NOT EDIT BY HAND"). Good.

I'd suggest adding a CI/pre-commit check that greps for hand edits by comparing the checked-in .S against a fresh regeneration from HEAD's intrinsics TU (or at least a regen-diff run recorded in the PR description). Follow-up nit.

Comment on the -O2 vs -O3 choice in the generator

Author defaults to -O2:

Default matches how ORT actually builds these sources (RelWithDebInfo is -O2). Freezing at -O3 measured 7-8% SLOWER at M == 1 on X925 — the single-row path has too little work to absorb -O3's extra unrolling — while being flat at M > 1, so -O2 is both the consistent and the faster choice. Re-measure before changing it.

Solid rationale (perf measurement + reproducibility). Good to spell it out for future maintainers.

NEON smmla/ummla packer simplification

vextq_s32 + vaddq_s32 + vdups_laneq_s32 compound reduction → single vpadd_s32(vget_low_s32(x), vget_high_s32(x)). Same operation, less code, portable NEON. Author's commit bae17bc ("portable RowSums reduction in the mmla packers"). Both signed and unsigned packers cleaned in parallel — qgemm_kernel_smmla.cpp, qgemm_kernel_ummla.cpp. Not perf-relevant (this is pack-time), but reduces intrinsic surface. ✓

Also fixes a pre-existing typo "zero pdding" → "zero padding". Nice attention to detail.

Windows dispatch state

Author's commit sequence had eb9e6d8 "[MLAS] Enable SVE QGEMM on Windows ARM64" followed by 8810dc7 "Revert '[MLAS] Enable SVE QGEMM on Windows ARM64'". So this PR does not flip the switch on Windows — the Windows CI leg presumably isn't ready. But the plumbing is here:

  • cpuid_info.cc adds SDK-guarded PF_ARM_SVE_I8MM_INSTRUCTIONS_AVAILABLE detection.
  • CMake setup_mlas_source_for_windows() wires the SVE QGEMM sources into the mlas_platform_preprocess_srcs list (cl /P + armasm64 pipeline).

Once Windows CI is confirmed, flipping is one platform.cpp change (drop the #if defined(__linux__) around the S8S8/U8X8 NEON smmla dispatch — or, more likely, register the SVE dispatch on Windows too). Deferrable, but call out in the release notes what is / isn't enabled.

U8U8 routing

Nice catch. The SVE ummla kernel is registered on both GemmU8U8Dispatch and GemmU8S8Dispatch, following the NEON precedent: MlasGemmQuantFixupZeroPointB<...UMMLA_SVE> bit-flips signed B → unsigned via XOR 0x80 and compensates via the zero-point fixup, so U8U8 is just the simpler case (no fixup) of the same kernel type. Commit 6bc26bb added the routing; commit 29e7f1f added the missing U8U8 benchmark captures (previously the entire U8U8 dispatch had zero benchmark coverage). Good hygiene.

BTI landing pads

Commit 61ba9a4 regenerated with BTI landing pads (KAI_ASM_BTI_C at each function entry). Correct for platforms with BTI enforcement enabled (bti c at every indirect call target). Consistent with KleidiAI's assembly style.

Concerns / follow-ups

  1. Windows enablement not in this PR. Documented in the description ("Revert 'Enable SVE QGEMM on Windows ARM64'"). Merges cleanly, but track the follow-up.
  2. CI regen check. Add a job that re-runs the freeze pipeline against HEAD's intrinsics reference TU and asserts byte-equality with the checked-in .S. Prevents accidental drift between the reference and the frozen build.
  3. Reserved-register list is ("x18",) — a tuple. If the future needs to add x27 (for some new platform), extending the tuple + regenerating from the intrinsics reference will catch it in CI. Correct design; nothing to change now.
  4. The 12x8 tile is guarded by MLAS_SVE_QGEMM_TILE_12X8 compile-define (default on via CMake). If someone builds without it, Rows never becomes 12 and the CopyPackA still emits an [8-group][4-group] pair — but the compute drops it. The header comment notes this: "12 rows are consumable only when CopyPackA's greedy 8/4/2/1 grouping emitted an [8-group][4-group] pair". If MLAS_SVE_QGEMM_TILE_12X8 is off and MLAS_SVE_QGEMM_TILE_8X12 is also off, CountM=12 falls into the Rows=8 tile which does not consume the second 4-group. Have you measured the wire-off case? If it's dead code today with both flags on, consider making the macros consistent-on-by-default via #ifndef fallback in the header rather than relying on compile-flags in CMake. Non-blocking.
  5. The scaling bound comment ("scales fully through VL=512 and stays correct at any larger VL... going beyond would need a cross-segment reduction at the end") is honest and correct. Future SVE2 targets with VL=1024 or 2048 will silently work but not scale further from VL=512. Fine — the next SVE2-specific kernel with cross-segment reduction can supersede.

Recommendation

Approve. This is a substantial piece of engineering:

  • The single-source compute (intrinsics reference) + frozen-bytes deployment pattern is exactly the right structure for a kernel that must ship on all platforms including toolchains without SVE support.
  • The x18 ABI fix is precisely correct in scope and enforcement: compiler flag + generator audit + documented rationale in both places.
  • The vector-length-agnostic scaling is mathematically proven by the segment/k-block bound, and empirically verified on VL=128 and VL=256.
  • Numbers land at parity or better than NEON on the 76-cell benchmark suite; 33213 tests pass; bit-identical to NEON via exact integer accumulation over the same packed operands.
  • The M=1 lean path is well-argued and delivered parity where earlier delegation was up to 1.07x slower.

Suggested follow-ups (non-blocking):

  • CI regen check to guarantee frozen .S stays byte-equal to the intrinsics reference.
  • Track Windows enablement as a separate PR when CI is ready.
  • Consider auto-defaulting MLAS_SVE_QGEMM_TILE_{12X8,8X12} in the header via #ifndef fallback so the 12-row tile isn't silently dropped in unusual build configs.

@hariharans29

Copy link
Copy Markdown
Member

Re-review — PR #31146: [ARM] MLAS: SVE i8mm (svmmla) int8 QGEMM kernels

One new commit since the previous review: 6e7f11b"SVE QGEMM: default the 12-row tile in the shared header". Directly addresses concern #4 from the prior review.

Follow-up #4 (macro-default location) — addressed with a strictly better fix than I proposed

Prior state: MLAS_SVE_QGEMM_TILE_12X8 was defined via -DMLAS_SVE_QGEMM_TILE_12X8=1 in four separate places in cmake/onnxruntime_mlas.cmake (Windows S/UMMLA sources; non-Windows S/UMMLA sources; the intrinsics reference build path — and the regen script docstring in gen_sve_asm.py). If a CMake refactor missed even one, the driver TU and the compute TU would silently disagree on whether the 12-row tile exists.

Commit 6e7f11b moves the default to the one header both TUs already include, qgemm_mmla_sve.h:

//
// The 12-row tile. The driver translation units use this to pick Strides.M, and
// the compute core uses it to pick the Rows == 12 path; the two are compiled
// separately, so the default lives in the one header both include rather than
// in per-target compile flags, where they could drift apart. An explicit
// -DMLAS_SVE_QGEMM_TILE_12X8=0 still overrides.
//
// Turning it off is correct, only slower: the kernel returns the row count it
// handled and the driver advances packed A linearly by it, so a 12-row group
// packed as [8-group][4-group] is simply consumed as 8 then 4.
//
#ifndef MLAS_SVE_QGEMM_TILE_12X8
#define MLAS_SVE_QGEMM_TILE_12X8 1
#endif

Two things this docstring does better than a plain #define would:

  1. Names the drift risk explicitly ("compiled separately, so the default lives in the one header both include rather than in per-target compile flags, where they could drift apart"). Future contributors thinking "I'll just move this back to CMake for parity with other flags" now have to justify undoing the fix.
  2. States the correctness invariant for =0 ("Turning it off is correct, only slower"). The 8-group + 4-group linear consumption is spelled out, so if someone bisects a perf regression to this flag they know it's a perf-only decision, not a correctness bomb. This is exactly the kind of comment that survives a decade of maintenance.

CMake removals: six set_source_files_properties(... -D...) lines gone across both build branches. Removals are consistent — Windows preprocess-srcs path, non-Windows srcs path (both SMMLA/UMMLA driver TUs), and the intrinsics reference build path. Gen-script docstring also cleaned up. -DMLAS_SVE_QGEMM_TILE_12X8=0 still works as an override because #ifndef respects it.

One follow-up question worth confirming before merge

The diff also removes the -DMLAS_SVE_QGEMM_TILE_8X12=1 from the same CMake lines and the same regen-script docstring, but I don't see a matching #ifndef MLAS_SVE_QGEMM_TILE_8X12 in qgemm_mmla_sve.h. Two possibilities:

  1. Dead-code cleanup: MLAS_SVE_QGEMM_TILE_8X12 was defined but never referenced by any #if (leftover from an earlier tile-shape experiment). Removing the -D is then pure hygiene. CI status (86/87 checks OK on this commit, same as before) is consistent with this reading, since any #if MLAS_SVE_QGEMM_TILE_8X12 block that got silently disabled would presumably fail an existing test.
  2. Actively-guarded code: if any #if defined(MLAS_SVE_QGEMM_TILE_8X12) && MLAS_SVE_QGEMM_TILE_8X12 block exists elsewhere in the SVE QGEMM sources, it's now silently disabled by default — same drift class this commit was written to prevent, just for the sibling macro.

Author: could you confirm whether MLAS_SVE_QGEMM_TILE_8X12 is dead code that was removed intentionally, or whether it needs a symmetric #ifndef ... #define ... 1 fallback next to the TILE_12X8 one? A one-line note in the PR conversation would resolve this — if dead code, saying so is fine; if not, the parallel fallback is a two-line addition.

Prior review's other suggestions — status

  • CI regen check. Still an open follow-up — no CI job yet that runs gen_sve_asm.py against HEAD's intrinsics reference and asserts byte-equality with the checked-in .S. Non-blocking; recommend as a separate PR.
  • Windows enablement. Still not flipped in this PR (revert commit 8810dc7 from earlier is still in effect). Plumbing is here; enablement stays deferred to when the Windows CI leg is ready.

Everything else previously verified — still verified

  • x18 ABI fix (compiler flag + generator audit + docs on both sides): unchanged. ✓
  • VL-agnostic scaling bound (Segments = min(VL, 64B) / 16B, capped at VL=512): unchanged. ✓
  • MlasQGemmSveSafeCol UB avoidance for tail columns: unchanged. ✓
  • M=1 lean path (dedicated single-row via MlasQGemmSveLoadASingleRowDup + svdup_n_s32 + skip-uzp2 store): unchanged. ✓
  • BTI landing pads in the frozen assembly: unchanged. ✓
  • U8U8 routing + benchmark coverage: unchanged. ✓
  • NEON smmla/ummla packer simplification (vpadd_s32 replacing the vext+vadd+vdups compound reduction): unchanged. ✓

Recommendation

Approve, pending author's one-line clarification on MLAS_SVE_QGEMM_TILE_8X12. If it's dead code that was cleaned up together with the 12X8 refactor, no further changes needed; if not, add a symmetric #ifndef ... #define MLAS_SVE_QGEMM_TILE_8X12 1 ... #endif next to the 12X8 one.

The pattern established by this commit — "when a macro coordinates a compile-time choice across separately-compiled translation units, the default belongs in the shared header, not in per-target compile flags" — is a genuinely reusable design principle for MLAS-style kernel setups. Worth calling out in a follow-up as a codebase convention.

@hariharans29

Copy link
Copy Markdown
Member

LGTM. Do we need smmla/svmmla integration into the MatmulNBits 4-bit and 8-bit paths as well ? Maybe that helps with prompt performance ?

@mirounga
mirounga merged commit a4156be into microsoft:main Aug 21, 2026
87 checks passed
@mirounga
mirounga deleted the rs_sve_qgemm branch August 24, 2026 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants