Skip to content

perf(fp8): Remove redundant clone calls from FP8 Linear Kernel - #841

Open
R3hankhan123 wants to merge 1 commit into
torch-spyre:mainfrom
R3hankhan123:redundant-clones
Open

perf(fp8): Remove redundant clone calls from FP8 Linear Kernel#841
R3hankhan123 wants to merge 1 commit into
torch-spyre:mainfrom
R3hankhan123:redundant-clones

Conversation

@R3hankhan123

Copy link
Copy Markdown
Contributor

Description

Remove Redundant clone calls from FP8 Linear Kernel

Related Issues

Fixes #840

Test Plan

curl request

curl http://localhost:8000/v1/chat/completions   -H "Content-Type: application/json"   -d '{
    "model": "ibm-granite/granite-4.1-8b-fp8",
    "messages": [
      {
        "role": "user",
        "content": "Hello! Explain what a transformer model is in one paragraph."
      }
    ],
    "max_tokens": 20,
    "temperature": 0.7
  }' | jq

output

"role": "assistant",
        "content": "A transformer model is a type of deep learning architecture designed to process and generate sequential data, such as",

Checklist

  • I have read the contributing guidelines
  • My code follows the project's code style (run bash format.sh)
  • I have added tests for my changes (if applicable)
  • I have updated the documentation (if applicable)
  • My commits include a Signed-off-by: line (DCO compliance)

@R3hankhan123
R3hankhan123 requested review from a team, dilipgb and yannicks1 as code owners September 10, 2026 12:25
@github-actions

Copy link
Copy Markdown
Contributor

👋 Hi! Thank you for contributing.
Just a reminder: Make sure that your code passes all the linting checks, otherwise your PR won't be able to be merged. To do so, run ./format.sh.
Now you are good to go 🚀.

We also recommend installing prek and configuring it to check your code before every local commit.

@tdoublep

Copy link
Copy Markdown
Collaborator

/caude-review

@tdoublep tdoublep left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Posted by Claude Code on behalf of @tdoublep.

I ran this on the device. The removed clones really are redundant: output is bitwise identical to main across 9 shape/M combinations, still contiguous at offset 0, and nothing reuses the buffer. Looks good.

Two notes:

  1. It also fixes a bug. On main, 3-D input that needs M padding comes back with wrong numbers (off by ~2-3 vs. the same rows passed as 2-D). Trimming before the reshape is what fixes it. Worth saying so in the description.
  2. Nothing tests this. test_scaled_mm_apply uses num_tokens 1/4/128, which all skip M padding, and it only checks dtype and shape — so the branch added here never runs in CI. Please add 5 or 130 and compare values.

Comment thread spyre_inference/custom_ops/fp8_linear_kernel.py
Comment thread spyre_inference/custom_ops/fp8_linear_kernel.py Outdated
Signed-off-by: Rehan Khan <Rehan.Khan7@ibm.com>
@joerunde

Copy link
Copy Markdown
Collaborator

/claude-review

Comment thread tests/test_fp8_linear.py
"""apply_weights runs aten._scaled_mm on Spyre.

num_tokens=5 and 130 exercise M-padding (not in _SMALL_M, not aligned
to _M_ALIGN=128), verifying the trim-before-reshape path.

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.

These two cases (5, 130) do add M-padding, but the test still only asserts dtype and shape and only ever passes 2-D x. The bug this PR fixes is 3-D input that needs M-padding: the pre-clone() reshape reads the padded storage and returns wrong numbers. On device I confirmed it — feeding the same 130 rows as (13, 10, 128) gives max|out3d - out2d| = 2.999 on main and 0.0 on this branch. Neither of those facts is checked here: no 3-D shape is exercised (so the reshape branch never runs in CI), and no value comparison is done (so even a 2-D numeric regression would pass). The docstring's claim that these cases are "verifying the trim-before-reshape path" is therefore not accurate. Please add a 3-D case and compare apply_weights output against a reference (e.g. the 2-D result reshaped, or a dequantized fp16 matmul), as tdoublep asked in the review.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@R3hankhan123 looks like this might need one more pass then we should be good to go

@github-actions

Copy link
Copy Markdown
Contributor

Reviewed on Spyre hardware. The clone removals in _join and the final return are safe, and moving the clone() to before the reshape is a real correctness fix, not just a perf change: feeding 130 rows as a 3-D (13, 10, 128) tensor gives max|out3d - out2d| = 2.999 on main versus 0.0 on this branch — the padded storage was corrupting the reshape on main. The fp8 tests run in the smoke shards on single-card (spyre_pf_x1) runners, which is correct for these 128×128 shapes, and all 16 cases pass.

One issue worth blocking on: the fix is untested. The new num_tokens 5/130 add M-padding but the tests still pass only 2-D x (so the reshape branch never runs in CI) and still assert only dtype/shape (so a numeric regression would pass). This is the value-comparison tdoublep asked for in the review; it hasn't been done yet. I left an inline comment with a concrete suggestion and one docstring-accuracy note. The two prior inline threads (the _join docstring and the block comment) are addressed — I resolved the first and left a one-line grammar nit on the second. Also worth doing per tdoublep's note: mention the bug fix in the PR description. Net: 1 significant gap (missing test coverage) plus 2 minor nits.

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.

Remove Redundant Clone calls for FP8 Linear Kernel

3 participants