Skip to content

cp_fp_reg_edges: honor _frm suffix on fs2 and fs3 generators#1554

Closed
Aditya Amol Kuchekar (dev-aditya-hub) wants to merge 1 commit into
riscv:act4from
dev-aditya-hub:fix/fp-edges-frm-symmetry
Closed

cp_fp_reg_edges: honor _frm suffix on fs2 and fs3 generators#1554
Aditya Amol Kuchekar (dev-aditya-hub) wants to merge 1 commit into
riscv:act4from
dev-aditya-hub:fix/fp-edges-frm-symmetry

Conversation

@dev-aditya-hub

Copy link
Copy Markdown
Contributor

Summary

make_fs1_edges in cp_fp_reg_edges.py honors a _frm suffix on the coverpoint name and crosses each edge value with all six rounding modes. make_fs2_edges and make_fs3_edges ignore the suffix and emit one frm-unspecified test per edge value.

Problem

Because the registry uses longest-prefix matching, a coverpoint named cp_fs2_edges_frm routes to make_fs2_edges, which silently drops the rounding-mode cross. The label says the suite covered the cross-product; the generated assembly did not. A DUT with a rounding-path bug that only triggers for specific fs2 or fs3 edge values (e.g., subnormal fs2 with RTZ on fmadd.s) is never exercised, while the symmetric fs1 case is.

Fix

Mirror the cross_frm / frm_modes handling from make_fs1_edges into make_fs2_edges and make_fs3_edges, including the bin-name and description suffixes so the generated labels stay distinct per frm mode.

Risk

Low. When the coverpoint does not end in _frm, frm_modes = [None] and the new loop runs exactly once per edge value with frm=None, producing the same output as before. Only coverpoint names containing _frm are affected.

Copilot AI left a comment

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.

Pull request overview

This PR fixes a gap in the floating-point coverpoint generators where cp_fs2_edges* and cp_fs3_edges* variants containing _frm were not actually crossing edge-value tests with all rounding modes, despite the coverpoint name implying that behavior. It aligns make_fs2_edges/make_fs3_edges behavior with the already-correct make_fs1_edges implementation.

Changes:

  • Add _frm detection to make_fs2_edges and make_fs3_edges and iterate edge values across ("dyn", "rdn", "rmm", "rne", "rtz", "rup") when requested.
  • Make bin names and descriptions frm-specific so per-frm generated cases remain distinct and accurately labeled.

@dev-aditya-hub

Copy link
Copy Markdown
Contributor Author

Heads-up on landing order for this one. I have two other PRs touching the same file/concept:

Happy to sequence however you prefer. My read is #1469 first (pure refactor, no behavior change), then #1554, then #1467 on top of the deduped helper. But I'll defer to whatever order makes the review easier.

@davidharrishmc

Copy link
Copy Markdown
Collaborator

I am concerned this change skips the tests entirely for cp_fs2_edges (without _frm) because frm_modes is set to None, and then the for loop doesn't iterate over any modes and doesn't print anything. Could be mistaken, but hard to be certain without looking at the emitted code.

The floating-point tests are placeholders and will be superseded this summer by more comprehensive tests based on the IBM coverpoints. Not sure that it's worth investing more time in tweaking and reviewing the placeholders.

@dev-aditya-hub

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

Copy link
Copy Markdown
Contributor Author

On (1): frm_modes is [None] (a 1-element list), not None the loop runs once with frm_mode = None, producing the same single test chunk per edge value as the previous make_fs2_edges. The cp_fs2_edges (no _frm) output is byte-identical to pre-refactor happy to attach a diff of generated tests if it'd help confirm.

On (2): fair point. #1469 (the refactor) already merged, so the shared helper is in tree. This PR is just the small follow-up that lets fs2/fs3 actually honor the _frm suffix (today they silently fall through to a single frm mode). If the IBM-coverpoint replacement is landing soon and you'd rather not carry even this small delta, happy to close your call.

@jordancarlin

Copy link
Copy Markdown
Collaborator

As far as I can tell, this was fully done in #1469. Please update this PR if there is anything else that actually needs to change. For now, I’m going to close it.

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.

4 participants