Skip to content

[XNNPACK] Test addmm with non-default alpha or beta - #24814

Closed
JakeStevens wants to merge 1 commit into
pytorch:mainfrom
JakeStevens:xnnpack-addmm-alpha-beta
Closed

JakeStevens wants to merge 1 commit into
pytorch:mainfrom
JakeStevens:xnnpack-addmm-alpha-beta

Conversation

@JakeStevens

@JakeStevens JakeStevens commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor

Summary

This PR started as a fix for the XNNPACK partitioner and the addmm-to-linear transform dropping addmm alpha and beta. #24658 landed the same fix on main, so after rebasing, this PR only adds tests.

test_convert_to_linear.py::test_fp32_addmm_with_alpha_or_beta runs an addmm with a transposed weight through ConvertToLinearPass, which extension/llm/export/builder.py runs on the whole program. It checks that only the default addmm becomes a linear and that each output matches eager, for only alpha, only beta, and beta=0. test_linear.py::test_fp32_addmm_with_alpha_or_beta checks that the partitioner leaves the same addmm on portable. They overlap with #24658's test_only_an_addmm_without_scales_becomes_a_linear and test_fp32_addmm_with_scales; what they add is beta=0 and the ConvertToLinearPass path.

Test plan

python -m pytest backends/xnnpack/test/passes/test_convert_to_linear.py

python -m pytest backends/xnnpack/test/ops/test_linear.py -o addopts="" -k addmm

The first test fails with the alpha and beta checks removed from addmm_can_be_linear, and the second with them removed from AddmmConfig.check_constraints. pytest.ini skips test_linear.py, hence -o addopts="".

This PR was authored with Claude Code.

@pytorch-bot

pytorch-bot Bot commented Nov 9, 2026 •

Copy link
Copy Markdown

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/24814

Note: Links to docs will display an error until the docs builds have been completed.

⏳ No Failures, 106 Pending

As of commit 8855927 with merge base bc5bd66 (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 9, 2026
@JakeStevens

Copy link
Copy Markdown
Contributor Author

@claude review effort:max

@claude

claude Bot commented Nov 9, 2026 •

Copy link
Copy Markdown

Claude finished @JakeStevens's task in 3m 17s —— View job


The partitioner check is correct, but in the LLM export flow a scaled addmm with a transposed weight still gets rewritten to linear, and alpha/beta are dropped there too. That leaves the wrong-result bug partly unfixed.

Detailed review

PR Review: #24814

Summary

AddmmConfig.check_constraints now rejects addmm when alpha or beta is not 1, so XNNPACK no longer delegates a node it would compute wrong. That part is correct. However, one remaining rewrite to linear still drops alpha/beta for nodes that are no longer delegated, so the PR does not fully deliver on "the addmm runs on the portable kernels". Three independent reviewers looked at both changed files. I re-read every finding below against the code. Nobody could run the tests in this environment.

Code Quality

  • alpha/beta are still dropped after partitioning, in backends/transforms/addmm_mm_to_linear.py:119-131. replace_addmm_mm_with_linear turns any edge.aten.addmm whose args[2] is t_copy/permute_copy into aten.linear(args[1], weight, args[0]) and never reads node.kwargs.
    • How it is reached: extension/llm/export/builder.py:558 runs ConvertToLinearPass() on the edge program after to_edge_transform_and_lower. That pass always falls back to apply_addmm_mm_to_linear_transform (backends/xnnpack/_passes/convert_to_linear.py:192). So the non-delegated addmm this PR leaves behind gets rewritten anyway.
    • Concrete trigger: an LLMEdgeManager export containing torch.addmm(b, x, W.t(), alpha=2, beta=3). XNNPACK now rejects the node, the builder then converts it to a plain linear, and the output is still x @ W.t() + b.
    • Wider impact: Vulkan (vulkan_preprocess.py:13) and Samsung (enn_pass_manager.py:26) use the same AddmmToLinearTransform, so they likely have the same problem. I have not checked whether their partitioners filter on alpha/beta.
    • Suggested fix: skip addmm nodes with non-default alpha/beta in replace_addmm_mm_with_linear. That is the root cause the PR description names. Doing it here or in a follow-up is fine, since this is pre-existing rather than a regression. Fix this →
  • Misleading message at backends/xnnpack/partition/config/gemm_configs.py:484. "XNNPACK does not support addmm with alpha or beta" sounds like every addmm is rejected. The Add/Sub configs (generic_node_configs.py:228, :867) say "doesn't support alpha != 1". Something like "XNNPACK does not support addmm with alpha != 1 or beta != 1" would match them.

Testing

  • No test covers the transposed-weight form, at backends/xnnpack/test/ops/test_linear.py:785-799. AddMMModule.mat is [in, out] and used untransposed (line 98). The shared transform therefore skips this node as a "standalone addmm" (addmm_mm_to_linear.py:121-126), so the test never reaches the code that actually drops the scalars. Adding a variant that uses self.mat.t() would cover the linear-like pattern.
  • Only the combined case is tested. The test always sets alpha=2, beta=3 together. If the check regressed to look at only one of the two (for example or changed to and), the test would still pass. Parametrizing alpha-only and beta-only cases would catch that. beta=0 is a good one, since it means "ignore input".
  • The new test does not run in OSS CI. pytest.ini:15 and pytest-windows.ini:55 --ignore the whole of test_linear.py. This is pre-existing and noted in the PR description, but it means this regression guard only runs internally or when invoked by hand.

Recommendation

Approve

The change is correct and strictly improves on main. The addmm_mm_to_linear hole should be fixed, either here or as a tracked follow-up, before the bug can be called fixed for LLM exports. Neither the hole nor the test gaps block this PR.

@JakeStevens JakeStevens added the release notes: xnnpack Changes to the XNNPack backend delegate label Nov 9, 2026
@JakeStevens
JakeStevens force-pushed the xnnpack-addmm-alpha-beta branch from 2c560b1 to fa8a870 Compare November 10, 2026 13:47
@JakeStevens JakeStevens changed the title [XNNPACK] Reject addmm with non-default alpha or beta Stop lowering addmm with non-default alpha or beta to linear Nov 10, 2026
pytorch#24658 made the XNNPACK partitioner reject an addmm whose alpha or beta is
not 1, and made the addmm-to-linear transform keep such an addmm. These
tests also cover ConvertToLinearPass, which the LLM export builder runs on
the whole program, and beta=0, which makes addmm ignore its input.
ConvertToLinearPass keeps an addmm with a transposed weight and alpha or
beta, and still turns the default one into a linear. The partitioner leaves
an addmm with only alpha, only beta, or beta=0 on portable.

This change was authored with Claude Code.
@JakeStevens JakeStevens changed the title Stop lowering addmm with non-default alpha or beta to linear [XNNPACK] Test addmm with non-default alpha or beta Nov 10, 2026
@JakeStevens
JakeStevens force-pushed the xnnpack-addmm-alpha-beta branch from fa8a870 to 8855927 Compare November 10, 2026 14:12
@JakeStevens

Copy link
Copy Markdown
Contributor Author

Dropping this: #24658 landed the same partitioner check and transform fix on main, with tests for these cases. #24352 no longer depends on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. release notes: xnnpack Changes to the XNNPack backend delegate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant