Skip to content

fix: support optional C in Gemm - #870

Draft
voltjia wants to merge 2 commits into
masterfrom
fix/align-gemm-with-onnx-semantics
Draft

fix: support optional C in Gemm#870
voltjia wants to merge 2 commits into
masterfrom
fix/align-gemm-with-onnx-semantics

Conversation

@voltjia

@voltjiavoltjia commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Implement non-null optional C for the Gemm signature introduced by fix!: align Gemm public signature #878.
  • Support independent and Y-broadcastable C across CPU, CUDA-family, Cambricon, Ascend, and PyTorch implementations.
  • Keep the existing parameter matrix while changing it to independent C/Y, and add focused C=None and broadcast-C smoke coverage.

Motivation

#878 changes the public Gemm signature immediately and temporarily rejects non-null C. This follow-up completes the aligned contract without mixing the larger backend implementation into the API-only review.

Depends on #878.

Type of Change

  • feat - new feature / new operator / new platform
  • fix - bug fix
  • perf - performance improvement (no behavioral change)
  • refactor - code restructuring without behavior change
  • test - adding or fixing tests only
  • docs - documentation only
  • build / ci - build system or CI configuration
  • chore - tooling, formatting, or other non-code changes
  • Breaking change (owned by fix!: align Gemm public signature #878)

Platforms Affected

  • CPU (WITH_CPU)
  • NVIDIA (WITH_NVIDIA)
  • Iluvatar (WITH_ILUVATAR)
  • MetaX (WITH_METAX)
  • Cambricon (WITH_CAMBRICON)
  • Moore (WITH_MOORE)
  • Ascend (WITH_ASCEND)
  • Hygon (WITH_HYGON, PyTorch implementation)
  • PyTorch C++ bindings (WITH_TORCH)
  • Build system / CMake / CI
  • Python bindings / user-facing API

Smoke Test Result

Exact-head build, Ruff, and both clang-format checks passed for 256fcad. Platform unit workflows are not currently listed for this stacked-head update.

The implementation files remain aligned with the previously validated #870 implementation. The stack rewrite changes the parent interface commit and keeps all newly added test cases in this implementation PR.

Previous #870 implementation validation:
NVIDIA, Iluvatar, MetaX, Moore, Cambricon, Ascend - main CI passed
NVIDIA, Iluvatar, MetaX, Moore, Cambricon, Ascend - shadow CI passed
Current stacked head:
python -m py_compile tests/test_gemm.py scripts/run_host_overhead_control.py
passed
python scripts/generate_wrappers.py --devices cpu --ops gemm --strict-ops
passed
git diff --check b33fccd9..256fcadb
passed

Test Results on Supported Platforms

PlatformAffectedBuild / Smoke ResultFull Result / Notes
NVIDIAYesExact-head platform job not listed; previous implementation passedNative BLAS, cuBLASLt, and Torch paths
IluvatarYesExact-head platform job not listed; previous implementation passedShared BLAS plus platform Add
MetaXYesExact-head platform job not listed; previous implementation passedShared BLAS plus platform Add
CambriconYesExact-head platform job not listed; previous implementation passedCNNL matmul plus op-tensor accumulation
MooreYesExact-head platform job not listed; previous implementation passedShared BLAS plus platform Add
AscendYesExact-head platform job not listed; previous implementation passedNative addmm/baddbmm input/output separation
HygonYesNot separately configured in this CI matrixPyTorch implementation only
CPUYesExact-head build passed; previous implementation validatedNative and PyTorch implementations
Focused test notes
The inherited Gemm matrix now passes distinct C and Y tensors without adding
new parameter combinations. One smoke test covers both no-C public overloads,
and one float32 smoke test covers broadcast C.

Benchmark / Performance Impact

No benchmark was run. Independent or broadcast C requires a post-GEMM elementwise accumulation on classic BLAS and CNNL paths. Omitted C remains a single GEMM with beta zero.

Notes for Reviewers

  • Review this PR relative to fix!: align Gemm public signature #878. The stacked diff is 13 implementation/test files and contains no documentation, example, or host-overhead script changes.
  • The four platform Add includes are required because the shared BLAS template calls each platform's existing Add::Call specialization for non-null C. No Add implementation file is modified.
  • Documentation remains unchanged: every current document and runnable example uses the still-valid three-tensor overload owned by fix!: align Gemm public signature #878.
  • C may be independent and broadcastable to Y. C/Y aliasing is rejected because staged backends overwrite Y before the Add step.
  • The existing Cartesian Gemm matrix now covers independent C/Y across its existing shapes, strides, dtypes, alpha/beta values, transposes, and implementations.
  • The no-C overload and broadcast-C smoke tests live here rather than in the interface-only fix!: align Gemm public signature #878.
  • The C=None smoke case verifies beta is ignored and an existing NaN-filled Y is not read.
  • The broadcast smoke case computes its expected result before invoking Gemm, so accidental mutation of input C cannot affect the reference.
  • No deprecated compatibility overload is retained.

@voltjia
voltjiaforce-pushed the fix/align-gemm-with-onnx-semantics branch 4 times, most recently from 6fb4a68 to e634686CompareAugust 4, 2026 08:38
@voltjiavoltjia mentioned this pull request Aug 4, 2026
19 tasks
@voltjia
voltjiaforce-pushed the fix/align-gemm-with-onnx-semantics branch from e634686 to 0afa1e4CompareAugust 4, 2026 09:38
@voltjiavoltjia changed the title fix!: align Gemm with ONNX semanticsfix: support optional C in GemmAug 4, 2026
@voltjia
voltjia changed the base branch from master to fix/align-gemm-signatureAugust 4, 2026 09:39
BREAKING CHANGE: Gemm now accepts optional C before attributes and writes Y; non-null C is not implemented yet.
@voltjia
voltjiaforce-pushed the fix/align-gemm-signature branch from 15dd340 to b33fccdCompareAugust 5, 2026 00:12
@voltjia
voltjiaforce-pushed the fix/align-gemm-with-onnx-semantics branch from 0afa1e4 to 256fcadCompareAugust 5, 2026 00:12
@voltjia
voltjiaforce-pushed the fix/align-gemm-signature branch 6 times, most recently from cec9c4a to 65923dcCompareAugust 5, 2026 07:12
Base automatically changed from fix/align-gemm-signature to masterAugust 5, 2026 07:28
Sign up for freeto 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.

1 participant

@voltjia