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

@voltjia voltjia 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

Platform Affected Build / Smoke Result Full Result / Notes
NVIDIA Yes Exact-head platform job not listed; previous implementation passed Native BLAS, cuBLASLt, and Torch paths
Iluvatar Yes Exact-head platform job not listed; previous implementation passed Shared BLAS plus platform Add
MetaX Yes Exact-head platform job not listed; previous implementation passed Shared BLAS plus platform Add
Cambricon Yes Exact-head platform job not listed; previous implementation passed CNNL matmul plus op-tensor accumulation
Moore Yes Exact-head platform job not listed; previous implementation passed Shared BLAS plus platform Add
Ascend Yes Exact-head platform job not listed; previous implementation passed Native addmm/baddbmm input/output separation
Hygon Yes Not separately configured in this CI matrix PyTorch implementation only
CPU Yes Exact-head build passed; previous implementation validated Native 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
voltjia force-pushed the fix/align-gemm-with-onnx-semantics branch 4 times, most recently from 6fb4a68 to e634686 Compare August 4, 2026 08:38
@voltjia voltjia mentioned this pull request Aug 4, 2026
19 tasks
@voltjia
voltjia force-pushed the fix/align-gemm-with-onnx-semantics branch from e634686 to 0afa1e4 Compare August 4, 2026 09:38
@voltjia voltjia changed the title fix!: align Gemm with ONNX semantics fix: support optional C in Gemm Aug 4, 2026
@voltjia
voltjia changed the base branch from master to fix/align-gemm-signature August 4, 2026 09:39
voltjia added 2 commits August 5, 2026 08:07
BREAKING CHANGE: Gemm now accepts optional C before attributes and writes Y; non-null C is not implemented yet.
@voltjia
voltjia force-pushed the fix/align-gemm-signature branch from 15dd340 to b33fccd Compare August 5, 2026 00:12
@voltjia
voltjia force-pushed the fix/align-gemm-with-onnx-semantics branch from 0afa1e4 to 256fcad Compare August 5, 2026 00:12
@voltjia
voltjia force-pushed the fix/align-gemm-signature branch 6 times, most recently from cec9c4a to 65923dc Compare August 5, 2026 07:12
Base automatically changed from fix/align-gemm-signature to master August 5, 2026 07:28
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.

1 participant