Skip to content

[MLAS][KleidiAI] Apply fp16 split independent fixes#28720

Merged
hariharans29 merged 2 commits into
microsoft:mainfrom
Laan33:fp16-split/01-independent-fixes-squashed
Jun 2, 2026
Merged

[MLAS][KleidiAI] Apply fp16 split independent fixes#28720
hariharans29 merged 2 commits into
microsoft:mainfrom
Laan33:fp16-split/01-independent-fixes-squashed

Conversation

@Laan33

@Laan33 Laan33 commented May 29, 2026

Copy link
Copy Markdown
Contributor

Description

This PR splits out a small set of independent fixes and test-scope updates from the larger CPU fp16 enablement work.

Changes:

  • Adds KleidiAI Conv2D regression coverage for LHS indirection cache behaviour when the per-thread padding buffer grows.
  • Runs that regression in both short and long Conv2D test paths when KleidiAI convolution is available.
  • Disables WebGPU for the fp16 Attention test case that uses mask_index, which WebGPU Attention does not support.
  • Renames the dynamic QGEMM batch-count parameter from BatchSize to BatchN for consistency with MLAS batch naming.

Motivation and Context

These changes are independent from the wider fp16 MLAS/API and CPU EP enablement work in #28487.

The Conv2D regression exercises the small-CI -> large-CI -> small-CI sequence that can expose stale cached LHS pointer tables after the KleidiAI padding buffer grows. The Attention test exclusion keeps the test targeted to providers that support the tested mask_index input combination.

Note: This PR is the first offshoot from #28487.

Squash the fp16 split independent fixes into a single IP review patch while preserving the original branch history separately.

Signed-off-by: Cathal Lawlor <cathal.lawlor@arm.com>
@Laan33 Laan33 changed the title Apply fp16 split independent fixes [MLAS][KleidiAI] Apply fp16 split independent fixes May 29, 2026
@hariharans29
hariharans29 requested a review from Copilot June 1, 2026 23:30

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

Splits a small set of independent fixes from the larger Arm64 fp16 MLAS/KleidiAI enablement effort: a refactored KleidiAI Conv2D regression test, a WebGPU exclusion for an unsupported fp16 Attention case, and a parameter rename for naming consistency.

Changes:

  • Extracted the KleidiAI LHS-pointer-cache-by-pad-growth regression into a named helper, gated on USE_KLEIDIAI, and invoked it from both ExecuteShort and the existing long-test location.
  • Disabled WebGPU for AttentionBatch1_Float16 since WebGPU Attention does not support mask_index.
  • Renamed BatchSize parameter to BatchN in ArmKleidiAI::MlasDynamicQGemmBatch to match the public MlasDynamicQGemmBatch declaration in mlas.h.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
onnxruntime/test/mlas/unittest/test_conv2d.h Factor KleidiAI pad-growth regression into a helper and call it from short/long Conv2D tests; expand mlasi.h include guard to USE_KLEIDIAI.
onnxruntime/test/contrib_ops/attention_op_test.cc Disable WebGPU EP for the fp16 mask_index Attention test.
onnxruntime/core/mlas/lib/kleidiai/qgemm_kleidiai.cpp Rename BatchSizeBatchN to match declaration in mlas.h/mlasi_kleidiai.h.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@hariharans29

Copy link
Copy Markdown
Member

Review — PR #28720 (MLAS/KleidiAI fp16-split independent fixes)

Verdict: approve. This is a clean split of three unrelated, low-risk changes from the larger fp16 enablement work in #28487. Each is small, locally justified, and easy to verify.

What's here

  1. BatchSizeBatchN rename in ArmKleidiAI::MlasDynamicQGemmBatch to match the public MlasDynamicQGemmBatch declaration in mlas.h and mlasi_kleidiai.h. Pure cosmetic, zero behavioral change.
  2. Conv2D KleidiAI pad-growth regression extracted from the long-test body into a named helper TestKleidiAiLhsPointerCacheByPadGrowth(), gated on USE_KLEIDIAI, and invoked from both ExecuteShort() and the existing long-test call site. The include-guard for mlasi.h was widened from MLAS_TARGET_AMD64 to MLAS_TARGET_AMD64 || USE_KLEIDIAI to make GetMlasPlatform().MlasConvPrepareOverride visible on aarch64.
  3. WebGPU disabled for AttentionBatch1_Float16 because WebGPU Attention does not support the mask_index input. Plumbed via the existing disable_webgpu parameter on RunAttentionTest.

Things that are right

  • The Conv2D regression is now in ExecuteShort too, so default onnxruntime_mlas_test runs catch a future regression — previously it only ran in the long path. That's a real coverage win for issue [cpu] Loading certain models leads to global error state on M4 Max #26669.
  • The USE_KLEIDIAI gate on the helper body and the explicit MlasConvPrepareOverride == nullptr runtime check together make the new test a no-op on builds where KleidiAI is compiled in but not active. Correct layering.
  • The rename keeps the parameter type and ordering identical, so callers don't have to change.
  • The disable_webgpu plumbing reuses the existing 4-flag tail of RunAttentionTest rather than inventing new infrastructure.

Minor comments

1. #else return; #endif is dead. The function returns at the end either way:

void TestKleidiAiLhsPointerCacheByPadGrowth() {
#if defined(USE_KLEIDIAI)
    ...
#else
    return;   // ← unreachable; function ends immediately below
#endif
}

Drop the else branch entirely — leave the #if defined(USE_KLEIDIAI) block and let the function be empty otherwise. Stylistic only.

2. The regression-test comment migrated from the long-test body to the helper definition, but the new helper body has a much shorter version. The original comment explained the smaller-CI → larger-CI → smaller-CI sequence, the pad-buffer growth, the cache-invalidation hazard, and the issue link. The new helper comment is two sentences and drops the issue link. Please preserve the link to issue #26669 in the helper's comment — it's exactly the kind of context that gets lost otherwise.

3. for (int i = 0; i < 4; ++i) is unexplained at the helper site. The original code had a comment "Repeat a few times to increase the likelihood of triggering a reallocation". Keep that comment on the loop in the helper too.

4. The disable_webgpu flag is set via a positional argument at position 14 of RunAttentionTest. That call site is now:

RunAttentionTest(input_data, weight_data, bias_data, mask_index_data, output_data,
                 batch_size, sequence_length, hidden_size, number_of_heads, true /*use_float16*/,
                 false, false, 0, nullptr, nullptr,
                 AttentionMaskType::MASK_1D_KEY_SEQ_LEN, 0, 0,
                 false /*disable_cpu*/, false /*disable_cuda*/, false /*disable_dml*/, disable_webgpu);

Three positional falses before the one you actually care about. The /*disable_cpu*/ etc. inline comments are doing real work here — please don't lose them in a future cleanup. If RunAttentionTest ever grows another optional, consider a follow-up to convert these tail flags to a small struct AttentionTestDisableFlags { bool cpu, cuda, dml, webgpu; };.

5. The rename touches 7 lines in qgemm_kleidiai.cpp. Confirm there are no other callers or comments in the KleidiAI subtree that still say BatchSize for this function — grep_search for BatchSize in mlas/lib/kleidiai/ if you haven't already, to make sure header/impl consistency is complete.

Bottom line

Merge after the trivial dead-return cleanup and (ideally) restoring the issue-link comment on the Conv2D helper. Neither is a blocker. 84/85 CI checks passing.

Remove the dead non-KleidiAI return path from the pad-growth helper and
restore the issue link plus reallocation rationale for the repeated shape
sequence.

Signed-off-by: Cathal Lawlor <cathal.lawlor@arm.com>
@Laan33

Laan33 commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

There are no other uses for BatchSize in the KleidiAI subtree for the MlasDynamicQGemmBatch function.

@hariharans29

Copy link
Copy Markdown
Member

Re-review — PR #28720 (commit fbf60fc)

Verdict: approve once CI is green.

The new commit addresses all three review comments from the prior round:

  • Issue #26669 link restored in the helper's comment.
  • Loop-iteration rationale restored ("Repeat a few times to increase the likelihood of triggering a reallocation").
  • Dead #else return; removed — the helper is now #if defined(USE_KLEIDIAI) ... #endif with an empty fallback, which is the right shape.

No new code introduced, no new concerns. CI was at 2/67 at fetch time (early in the run); wait for it to complete, then merge.

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

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@hariharans29
hariharans29 enabled auto-merge (squash) June 2, 2026 17:43
@hariharans29
hariharans29 merged commit 1e2b639 into microsoft:main Jun 2, 2026
94 of 97 checks passed
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.

3 participants