Exclude x2 from randomly chosen rs2 in CSS-type stores - #2318
Closed
David Harris (davidharrishmc) wants to merge 1 commit into
Closed
David Harris (davidharrishmc) wants to merge 1 commit into
David Harris (davidharrishmc) wants to merge 1 commit into
Conversation
format_css_type loads rs2val into x{rs2} and then overwrites x2 with the
store base address via "addi sp, x{sig_reg}, -imm". When rs2 lands on x2
the loaded value is destroyed before the store executes and c.swsp/c.sdsp
writes the base address instead, so the testcase checks a signature slot
that has nothing to do with the value it claims to be testing.
Add excluded_regs={"rs2": {2}} to css_config. params.py applies operand
exclusions only when the operand is unset, so the cp_rs2 sweep, which
pins rs2=2 explicitly and for which the aliasing is inherent to the bin,
still emits its x2 testcase; only the random and edge-value paths stop
choosing x2.
a_type.py and ap_type.py are deliberately left alone. The issue reports
the same shape there, but those formatters use cmp_rs1_rs2_nx0, which
forces rs1 == rs2 on purpose; the redundant load_int_reg call is
cosmetic, not a defect.
Verification:
- rm -f work/stamps/*.stamp && make tests -j32
- Six Zca c.swsp/c.sdsp files changed across rv32i, rv32e, rv64i, rv64e.
- Before the fix, 15 testcases across 5 of those files stored x2 outside
the cp_rs2 sweep and were therefore dead, all under cp_imm_mul_4sp and
cp_imm_mul_8sp. After the fix there are 0.
- The cp_rs2 x2 bin survives in all six files, e.g. rv64i c.sdsp still
emits Zca_c_sdsp_cg_cp_rs2_b2 with "c.sdsp x2, 184(sp)".
- No cp_rs2_edges testcase uses x2. It happened not to draw x2 in the
checked-in baseline either, so this fix closes the hole rather than
repairing an observed miss on that coverpoint.
- uv run ruff check generators/ and ruff format --check both clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PTjX9BBLNAQkWwTTq6vfxq
Signed-off-by: David Harris <David_Harris@hmc.edu>
David Harris (davidharrishmc)
marked this pull request as ready for review
September 11, 2026 04:47
Copilot started reviewing on behalf of
David Harris (davidharrishmc)
September 11, 2026 04:48
View session
github-actions
Bot
added
status: needs-review
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Random CSS store cases can still select x0, discarding intended test values.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR excludes x2 from randomly selected CSS store source registers and regenerates affected tests.
Changes:
- Adds an
rs2exclusion forx2. - Regenerates CSS store test cases.
- Preserves explicit
cp_rs2coverage.
File summaries
| File | Summary |
|---|---|
tests/rv64i/Zca/Zca-c.sdsp-00.S |
Regenerated doubleword store tests. |
tests/rv32i/Zca/Zca-c.swsp-00.S |
Regenerated word store tests. |
tests/rv32e/Zca/Zca-c.swsp-00.S |
Regenerated RV32E word store tests. |
generators/testgen/src/testgen/formatters/types/css_type.py |
Excludes x2, but still permits x0 for random cases. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
59 tasks
Collaborator
|
I disagree that this is a bug. If the ISA allows storing the value in x2, we should test that. It stores a different value (the address), but that doesn't seem like an issue. It still lets us check if it is storing the right thing. Closing. David Harris (@davidharrishmc) feel free to reopen if you feel differently. |
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.
Issue #2147 item 25.
css_type.pyloadsrs2valintox{rs2}and then clobbers x2 with the store base address (addi sp, x{sig_reg}, -imm). Ifrs2 == 2the value is discarded and the store writes the address, so the testcase checks the wrong thing.Fix:
excluded_regs={"rs2": {2}}.params.pyapplies operand exclusions only when the operand is unset, so thecp_rs2sweep keeps its explicitrs2=2bin (where the aliasing is inherent) while random and edge-value picks stop landing on x2.Regenerated: 6 files. Before, 15 testcases across 5 files stored x2 outside
cp_rs2— all undercp_imm_mul_4sp/cp_imm_mul_8sp— and were silently dead; now 0. Thecp_rs2x2 bin survives in all six files, and nocp_rs2_edgestestcase uses x2.a_type.pyandap_type.pyare intentionally untouched: the issue flags the same shape, but there the aliasing is architecturally required (cmp_rs1_rs2_nx0forces rs1 == rs2), so only the redundantload_int_regline is cosmetic.🤖 Generated with Claude Code
https://claude.ai/code/session_01PTjX9BBLNAQkWwTTq6vfxq