bench(rocm,rope): use a real cos/sin cache in bench_rope - #270
Merged
Conversation
_shared_cos_sin_cache() built the cos||sin table with torch.randn, so values were unbounded and did not satisfy cos^2+sin^2=1. That made the --accuracy error numbers non-representative and let inplace runs drift, since repeated rotations were not norm-preserving. Build a proper RoPE cos||sin table instead (same construction as benchmarks/bench_rope.py / tests/test_helpers/rope_reference.py): entries are bounded in [-1, 1] and rotations are norm-preserving. Shape, dtype (float32) and ~32 MiB footprint are unchanged. Addresses Copilot review on PR #267 (discussion r3468044946). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes the ROCm RoPE benchmark’s cos/sin cache so it is a mathematically valid RoPE cos‖sin table (rather than random data), making the benchmark’s --accuracy output and repeated inplace-rotation behavior meaningful. It also updates internal PR-workflow documentation to reflect that the canonical repo is now AMD-Ecosystem/flashinfer.
Changes:
- Replace the ROCm benchmark’s random cos/sin cache initialization with a standard RoPE frequency-based construction (θ=1e4), matching existing reference implementations.
- Add a
_ROPE_THETAconstant to make the cache construction explicit and consistent. - Retarget PR-workflow docs and safeguards from
ROCm/flashinfertoAMD-Ecosystem/flashinfer.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
benchmarks/rocm_benchmarks/bench_rope.py |
Builds a correct RoPE cos‖sin cache (bounded, norm-preserving) for accurate benchmarking and comparisons. |
CLAUDE.md |
Updates PR-target guidance to the new AMD-Ecosystem/flashinfer repo slug. |
.claude/skills/pr-workflow/SKILL.md |
Retargets gh/API/GraphQL examples and safeguards to AMD-Ecosystem/flashinfer. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
demandal25
force-pushed
the
worktree-fix-rope-cos-sin-cache
branch
from
July 29, 2026 16:22
8bccfad to
1fd74c8
Compare
1 task
demandal25
changed the title
demandal25
added a commit
that referenced
this pull request
Jul 29, 2026
## Summary Repo and PR-workflow documentation updates, split out of #270 so that PR stays focused on the benchmark code change. ### What changed - **`CLAUDE.md`** — retarget the PR-target safeguard to `AMD-Ecosystem/flashinfer` (the repo moved from `ROCm/flashinfer`); add a "PR / issue body formatting" rule (GitHub renders a single newline inside a paragraph as `<br>`, so rendered bodies must not be hard-wrapped); add "Branch naming" and "CRITICAL: ask before pushing to remote" sections ported from the `AMD-Ecosystem/flashinfer-bench` pr-workflow skill. - **`.claude/skills/pr-workflow/SKILL.md`** — retarget the repo slug, `gh repo set-default` check, `gh api` paths, and GraphQL `owner` to `AMD-Ecosystem`. ## Test plan - [x] `pre-commit run --files CLAUDE.md .claude/skills/pr-workflow/SKILL.md` — markdownlint and the other hooks pass. - Docs-only change; no code paths touched, so no pytest applies. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the cos/sin cache in the ROCm RoPE benchmark so it holds a real cos‖sin table instead of random values.
What changed
benchmarks/rocm_benchmarks/bench_rope.py—_shared_cos_sin_cache()built the table withtorch.randn, so values were unbounded and did not satisfycos²+sin²=1. That made the--accuracyerror numbers non-representative and let inplace runs drift (repeated rotations were not norm-preserving). It now builds a proper RoPE cos‖sin table using the same construction asbenchmarks/bench_rope.py/tests/test_helpers/rope_reference.py(inv_freq = 1/θ^(2i/d), θ=1e4). Shape(65536, 128), dtype float32, and the ~32 MiB shared-tensor footprint are unchanged.This addresses the Copilot review on PR #267 (discussion r3468044946).
Test plan
(65536, 128), values in [-1, 1],max|cos²+sin²-1| ≈ 1.2e-7(float32 epsilon), ~32 MiB — matches the prior tensor's shape/dtype/size.pre-commit run --files benchmarks/rocm_benchmarks/bench_rope.py— all hooks pass.bench_rope.pyis a benchmark script (not in the test suite).