[MLAS] Add ARM64 NEON fp32 RoPE kernel - #29836
Conversation
MlasRopeDispatchNeon only ever assigned HRope, and only when FP16 vector acceleration is available; SRope was never assigned, so fp32 rotary embedding always fell back to the scalar reference implementation on ARM64 regardless of hardware. test_rope.cpp's shared test guard excluded MLAS_TARGET_ARM64 entirely, reflecting the same gap. Add a NEON fp32 kernel (non-interleaved and interleaved) and wire it into d.SRope unconditionally, outside the FP16 guard, since fp32 doesn't depend on FP16 vector support. The interleaved path uses vld2q_f32/vst2q_f32 to deinterleave/reinterleave the real/imag pairs directly instead of the shuffle/permute sequence the AVX2 implementation needs for the same case. Extend test_rope.cpp's ARM64 guard to register the existing 14 fp32 cases. fp16 stays AMD64/RVV-only, since ARM64 fp16 already has dedicated coverage in test_rope_neon_fp16.cpp.
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR adds an ARM64 NEON-optimized fp32 Rotary Positional Embedding (RoPE) kernel to MLAS and wires it into the ARM64 NEON dispatch path so MlasRotaryEmbedOneRow<float> no longer falls back to the scalar reference implementation on ARM64.
Changes:
- Implement fp32 NEON RoPE kernels (non-interleaved and interleaved) and expose them via
rope_neon::RopeKernel_Fp32. - Assign the fp32 kernel to
MlasRopeDispatchNeon.SRopeunconditionally (independent of fp16 acceleration support). - Enable the existing fp32 RoPE short-execute tests on ARM64 while keeping fp16 registration limited to AMD64/RVV (ARM64 fp16 covered elsewhere).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| onnxruntime/test/mlas/unittest/test_rope.cpp | Registers fp32 RoPE short-execute tests on ARM64; keeps fp16 registration on AMD64/RVV only. |
| onnxruntime/core/mlas/lib/rotary_embedding_kernel_neon.h | Declares the new fp32 NEON RoPE kernel entry point. |
| onnxruntime/core/mlas/lib/rotary_embedding_kernel_neon.cpp | Implements fp32 NEON RoPE (interleaved + non-interleaved) and hooks it into MlasRopeDispatchNeon.SRope. |
Review: PR #29836 — [MLAS] Add ARM64 NEON fp32 RoPE kernel (head
|
| dim | interleaved | scalar | new NEON | speedup |
|---|---|---|---|---|
| 128 | no | 514 ns | 24.4 ns | 21.1× |
| 256 | no | 1028 ns | 43.1 ns | 23.9× |
| 512 | no | 2067 ns | 80.6 ns | 25.6× |
| 1024 | no | 4161 ns | 156 ns | 26.7× |
| 128 | yes | 264 ns | 44.4 ns | 5.9× |
| 256 | yes | 521 ns | 85.0 ns | 6.1× |
| 512 | yes | 1035 ns | 165 ns | 6.3× |
| 1024 | yes | 2064 ns | 329 ns | 6.3× |
The non-interleaved speedup being ~4× larger than the interleaved case is expected: interleaved requires the deinterleave/reinterleave via vld2/vst2, which on Neoverse-N1 execute as multiple micro-ops vs. the single-cycle vld1/vst1 used by the non-interleaved path. Both wins are still substantial and well worth the change.
Minor observations (none blocking)
vmulq_f32 + vfmsq_f32 / vfmaq_f32pattern: one non-FMA multiply plus one FMA per output pair. An alternative would be to zero-initialize and use two pure FMAs, but that trades a multiply for a zero-materialize and doesn't help on ARMv8. The current idiom is what most NEON RoPE implementations use; the compiler often folds this into a pair of FMLAs anyway.#include <cassert>is added for the singleassert(dim % 2 == 0). Correct include for the header used. In release (NDEBUG), the assert is compiled out — fine, since the callers ensure the invariant.- Anonymous namespace for
RopeKernel_Fp32_Impl<...>with the public entry point exposed vianamespace rope_neon— clean internal/external linkage separation, matches the existing fp16 kernel structure in the same file. - No changes to
MlasRopeDispatchNeon's fp16 assignment: correct — this PR is deliberately scoped to fp32 only.
Bottom line
Clean, focused perf improvement. Real correctness gap (scalar fallback on all ARM64) closed. Benchmarks are convincing, tests are enabled where they should be, and the scoping is right. The single Copilot comment is a style-level indentation nit that doesn't affect correctness. Approve.
|
Please address the copilot comments |
Addresses Copilot review feedback on PR microsoft#29836.
Summary
ARM64 has no NEON implementation for fp32 rotary embedding.
MlasRopeDispatchNeononly ever assignsHRope, and only when FP16 vector acceleration is available;SRopeis never assigned, soMlasRotaryEmbedOneRow<float>always falls back to the scalar reference implementation on ARM64 regardless of hardware.test_rope.cpp's shared test registration guard reflects this too, excludingMLAS_TARGET_ARM64entirely.This adds a NEON fp32 kernel (
RopeKernel_Fp32, non-interleaved and interleaved variants) torotary_embedding_kernel_neon.cpp/.hand assigns it tod.SRopeunconditionally inMlasRopeDispatchNeon, outside theMlasFp16AccelerationSupported()guard since fp32 doesn't depend on FP16 vector support. The interleaved path usesvld2q_f32/vst2q_f32to deinterleave/reinterleave the real/imag pairs directly, instead of the shuffle/permute sequence the AVX2 implementation uses for the same case.test_rope.cpp's ARM64 guard now registers the existing 14 fp32 cases (dim 6/16/24/32/42/64/70 x interleaved). The fp16 half of that guard is left AMD64/RVV-only, since ARM64 fp16 already has its own dedicated coverage intest_rope_neon_fp16.cpp— this change is scoped to fp32 only.Testing
onnxruntime_mlas_test, full suite, no regressions:RoPE_fp32/*: 14/14 passed on both.Benchmark (Neoverse-N1, Oracle Cloud A1, median of 3 runs,
RoPE<float>)Numbers are from Neoverse-N1 only; it's a dedicated instance and gives a cleaner signal than a shared laptop.