Skip to content

frm coverpoints: sweep all 5 fcsr.frm values for dyn rounding mode#1467

Closed
Aditya Amol Kuchekar (dev-aditya-hub) wants to merge 1 commit into
riscv:act4from
dev-aditya-hub:fix/cp-frm-dyn-csr-leak
Closed

frm coverpoints: sweep all 5 fcsr.frm values for dyn rounding mode#1467
Aditya Amol Kuchekar (dev-aditya-hub) wants to merge 1 commit into
riscv:act4from
dev-aditya-hub:fix/cp-frm-dyn-csr-leak

Conversation

@dev-aditya-hub

@dev-aditya-hub Aditya Amol Kuchekar (dev-aditya-hub) commented May 9, 2026

Copy link
Copy Markdown
Contributor

Summary

For the dyn rounding-mode case, the four frm-bearing coverpoint generators previously called generate_random_params(..., frm=\"dyn\") without specifying csr_frm_val. formatters/params.py then filled it in with random_range(0, 4). That meant any individual instruction got exactly one randomly-chosen fcsr.frm value, and 1-in-5 of those landed on rne — which is also the power-on default — so a DUT that ignores fcsr.frm entirely would silently pass.

This PR replaces the single random pick with an explicit sweep of all five legal fcsr.frm values (0..4) for the dyn mode. Static modes are unchanged — csr_frm_val is left as None for them, which is identical to the old behavior since the formatter only consults it when frm == \"dyn\".

Change shape

The frm_modes tuple is replaced with a frm_variants list of (frm_mode, csr_frm_val) pairs:

  • 5 dyn variants — one per legal fcsr.frm value
  • 5 static variants — rdn, rmm, rne, rtz, rup, each with csr_frm_val=None

Bin names and descriptions include the fcsr.frm value when present, so the dyn entries stay distinct in the signature region.

Touched call sites:

  • cp_frm.pymake_frm
  • cp_fp_reg_edges.pymake_fs1_edges
  • cr_fp_reg_edges.pymake_cr_fs1_fs2_edges and make_cr_fs1_fs3_edges

What this replaces

The earlier revision of this PR pinned csr_frm_val=4 to defeat the rne-default collision. Jordan Carlin (@jordancarlin) pointed out that this gave up coverage of the other four legal values for the sake of catching one bug class. This revision keeps that bug class caught and restores deterministic coverage of all five values.

Cost

For each affected coverpoint, the dyn case grows from 1 testcase to 5. Static modes are unchanged. Net effect per coverpoint invocation: +4 testcases. The cross-product generators (cr_fs1_fs2_edges_frm, cr_fs1_fs3_edges_frm) feel this most, since the +4 multiplies through the edge × edge cross. If that growth is a concern, happy to gate the dyn-expansion behind a flag or trim the static-mode set.

Risk

  • For non-dyn modes, csr_frm_val=None is passed; identical to the prior behavior.
  • For dyn, the formatter now emits a deterministic fsrmi <csr_val> per testcase rather than a random one.
  • No changes to params.py or other shared infrastructure.

@jordancarlin

Copy link
Copy Markdown
Collaborator

Unless I am missing something, I think we want to test all 5 possible values for the dynamic rounding mode field. While 0 is the default, it is still a valid value that we want to make sure we test. With the randomization, we will be testing all 5 values, and I don't see a reason to constrain it to just an FRM value of 4.

@dev-aditya-hub Aditya Amol Kuchekar (dev-aditya-hub) changed the title Fix dyn frm coverpoints randomly picking rne (default), masking DUT bugs frm coverpoints: sweep all 5 fcsr.frm values for dyn rounding mode May 21, 2026
@dev-aditya-hub

Aditya Amol Kuchekar (dev-aditya-hub) commented May 21, 2026

Copy link
Copy Markdown
Contributor Author

Fair point pinning to 4 was throwing away coverage of the other four values just to defeat the rne-collision. I've reworked the patch (force-pushed 8860071) so that the dyn case is expanded into 5 explicit csr_frm_val variants (0..4) instead of being collapsed to one. That keeps deterministic coverage of every legal fcsr.frm value while still ensuring no test silently falls back to the rne default.
The static modes are unchanged. The only material cost is +4 testcases per coverpoint invocation in the dyn slot; for the cross-product generators that multiplies through the edge × edge cross, so let me know if that growth is a concern and I'll gate it or trim.

somil jain (somiljain2006) pushed a commit to somiljain2006/riscv-arch-test that referenced this pull request May 23, 2026
…riscv#1469)

## Summary

`make_fs1_edges`, `make_fs2_edges`, and `make_fs3_edges` in
`cp_fp_reg_edges.py` were near-duplicates. The `cross_frm` branch was
only ever added to fs1, so `cp_fs2_edges_frm` and `cp_fs3_edges_frm`
would silently fall through to a single frm mode if any testplan ever
opted into them.

Pulled the shared body into `_make_fs_edges(operand, ...)` and reduced
each public generator to a one-line dispatch. The helper handles
edge-set selection, the `_frm` cross, and the bin-name / description
formatting.

## Why

- Three copies of the same logic were drifting. fs1 already gained
`cross_frm` support; fs2 and fs3 didn't.
- One source of truth means the next change (new operand, new frm
behavior, new edge category) lands in one place by construction.

## Behavior

- `cp_fs1_edges*` — output unchanged.
- `cp_fs2_edges` / `cp_fs3_edges` (no `_frm`) — output unchanged;
`frm_mode=None` reproduces the same `bin_name` and `desc` as before.
- `cp_fs2_edges_frm` / `cp_fs3_edges_frm` — now functional; sweep all
six rounding modes, mirroring fs1.

## Notes

- This revision drops the `csr_frm_val=4` dyn-mode pin per
@jordancarlin's review comment. That change is being handled separately
on riscv#1467, where the discussion of how to address the random-rne
collision belongs.
- Net diff: -42 / +19 in the only file touched.
- No public API change; `add_coverpoint_generator` registrations are
unchanged.

---------

Co-authored-by: Aditya Amol Kuchekar <adityakuchekar0077@gmail.com>
Co-authored-by: David Harris <74973295+davidharrishmc@users.noreply.github.com>
BilalAli10x pushed a commit to BilalAli10x/riscv-arch-test that referenced this pull request May 25, 2026
…riscv#1469)

## Summary

`make_fs1_edges`, `make_fs2_edges`, and `make_fs3_edges` in
`cp_fp_reg_edges.py` were near-duplicates. The `cross_frm` branch was
only ever added to fs1, so `cp_fs2_edges_frm` and `cp_fs3_edges_frm`
would silently fall through to a single frm mode if any testplan ever
opted into them.

Pulled the shared body into `_make_fs_edges(operand, ...)` and reduced
each public generator to a one-line dispatch. The helper handles
edge-set selection, the `_frm` cross, and the bin-name / description
formatting.

## Why

- Three copies of the same logic were drifting. fs1 already gained
`cross_frm` support; fs2 and fs3 didn't.
- One source of truth means the next change (new operand, new frm
behavior, new edge category) lands in one place by construction.

## Behavior

- `cp_fs1_edges*` — output unchanged.
- `cp_fs2_edges` / `cp_fs3_edges` (no `_frm`) — output unchanged;
`frm_mode=None` reproduces the same `bin_name` and `desc` as before.
- `cp_fs2_edges_frm` / `cp_fs3_edges_frm` — now functional; sweep all
six rounding modes, mirroring fs1.

## Notes

- This revision drops the `csr_frm_val=4` dyn-mode pin per
@jordancarlin's review comment. That change is being handled separately
on riscv#1467, where the discussion of how to address the random-rne
collision belongs.
- Net diff: -42 / +19 in the only file touched.
- No public API change; `add_coverpoint_generator` registrations are
unchanged.

---------

Co-authored-by: Aditya Amol Kuchekar <adityakuchekar0077@gmail.com>
Co-authored-by: David Harris <74973295+davidharrishmc@users.noreply.github.com>
@jordancarlin

Copy link
Copy Markdown
Collaborator

The sweep through all of the legal fcsr.frm values is already done in the cp_csr_frm coverpoint. The cp_frm coverpoint should not duplicate it. The random value in fcsr.frm seems fine for this coverpoint. It provides a high-level check that the core respects dyn mode while cp_csr_frm checks it more extensively. I'm going to close this, but feel free to open an issue if you still think there is a hole here and we can discuss it in more detail.

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.

2 participants