Skip to content

Value class coverage - #97

Open
cwssemi wants to merge 3 commits into
chipsalliance:mainfrom
cwssemi:value-class-coverage
Open

Value class coverage#97
cwssemi wants to merge 3 commits into
chipsalliance:mainfrom
cwssemi:value-class-coverage

Conversation

@cwssemi

@cwssemi cwssemi commented Aug 25, 2026

Copy link
Copy Markdown

Extends the declared test values so that a handful of paths the current vectors do not
sensitise are exercised. Motivated by validating a RISC-V vector implementation, where a
couple of decode issues turned out to sit on exactly those paths, so the existing vectors
passed on them.

Three commits, and the third is comments only:

  1. extend value coverage for shifts, rotates, saturation and dividevror.vi at
    uimm >= 32 (its immediate is six bits, the top one funct6[0], so that encoding was
    unreached); the two ends of the simm5 range; operand pairs that reach the saturation
    boundary where none did; INT_MIN / -1; and an operand that separates a shift by 31
    from one by 63 where 0x0 and -1 cannot.

  2. add INT_MIN operands to the signed-arithmetic families — 118 configs. Scoped to
    where INT_MIN is the edge case, and deliberately not added to loads, stores, mask
    logic or permutes, where the column is an opaque bit pattern.

  3. comment the added value classes — 191 comments across 128 configs explaining why each value is
    there. The configs carry no comments today, so this is a departure from house style.
    It is a separate commit for exactly that reason — drop it if you would rather keep the
    configs as they are, and the first two stand unchanged.

What these values found

Run against a production RVV implementation (Saturn, ucb-bar/saturn-vectors) on the day they
were added, these configs surfaced two silent-wrong-result defects that the existing vectors
pass on
. Both were root-caused to one line of RTL each, both fixed and measured:

  • vror.vi at imm >= 32 shifts arithmetically. The rotate's shift half sign-extends,
    because the signedness flag is taken from funct6(0) — which means arithmetic for the shift
    family but direction for the rotate family. Reached only by the uimm >= 32 values in
    commit 1; the largest immediate the configs declared before was 31, so object RORI had never
    been executed by a generated test at any SEW.
  • vsmul saturates at 32-bit bounds when SEW=64. A Scala Int overflow in a Chisel
    constant: 1 << ((8 << sew)-1) masks the shift distance to 5 bits, so sew=3 clips at ±2^31.
    A genuine saturation returns INT32_MAX instead of INT64_MAX, and a result merely larger
    than INT32_MAX is clipped although the spec does not saturate it, raising vxsat spuriously.
    Reached only by the INT_MIN operands in commit 2.

Neither is exotic. INT_MIN × INT_MIN is the single defined saturation case for vsmul, and
vror.vi has a six-bit immediate of which the configs exercised five bits. Both are the kind of
value a coverage list is for, and in both cases the instruction was already "tested" — the
existing vectors execute it and pass.

Status

Opening as a draft so the repo's own CI runs it: build-and-test.yml does
make generate-stage1 then make all across VLEN 256/128/64 × XLEN 64/32. 128 configs is a wide
change and the goldens are the part worth checking, so I would rather that ran before asking anyone
to look. Ready for review once it is green.

Checks

  • Full generate-stage1 passes: 1,612 files, against 1,555 before. Byte-identical
    with and without the comment commit.
  • Value typing follows the existing convention — bare hex for base/sew8/sew16/
    sew32, quoted strings for sew64, matching the uint8/16/32 and string fields the
    decoder expects.
  • Row width matches each block; the multiply-accumulate forms carry three columns because
    vd is also an input.

Related

Separate from #94 and its PR, which
is a generator change restoring the declared sew64 blocks for the vd,vs2,uimm,vm and vd,vs2,vm
formats. These are config-only and independent, though the two compose: restoring that dimension is
what makes the SEW=64 values here reachable for those instructions.

…vide

Adds declared values that reach several paths the current vectors do not
sensitise. Motivated by validating a RISC-V vector implementation, where a
couple of decode issues turned out to sit on exactly those paths, so the
existing vectors passed on them.

  vror.vi      uimm 33, 48 and 63. vror.vi takes a SIX-bit immediate whose top
               bit is funct6[0], so imm >= 32 is a distinct encoding; the
               largest declared uimm was 31, leaving that encoding unexercised
               at every SEW. 33 and 63 are also non-self-inverse at every SEW,
               so the rotate direction is observable in the result.

  simm5 forms  15 and 16, which the (int8(v)<<3)>>3 re-encoding emits as +15 and
               -16, the two ends of the simm5 range. The declared set previously
               reduced to {0, 1, 3, -1}, all of which are unchanged by a 4-bit
               truncation of the field.

  vsadd/vssub  operand pairs that reach the saturation boundary at the SEWs
               where none previously did. vssub had no saturating pair at any
               SEW. Chosen to overflow under either operand order.

  vdiv/vrem    INT_MIN / -1 at all four SEWs -- the overflow case the spec calls
               out, whose result is defined rather than trapping.

  vsra/vssra   0x7fffffffffffffff at imm=31. The two declared operands at
  vnsra/vnclip imm >= 16 were 0x0 and -1, both fixed points of arithmetic right
               shift, so a shift by 31 and a shift by 63 give the same result on
               them; 0x7fff... separates the two.

Value typing follows the existing convention: bare hex for base/sew8/16/32 and
quoted strings for sew64, matching the uint8/16/32 and string fields the decoder
expects.
INT_MIN is the asymmetric end of two's complement -- -INT_MIN is not
representable, INT_MIN/-1 overflows, and it is the clamp target for the signed
saturating operations. It appeared as an operand in about 3% of configs at
SEW=64 and 8% at SEW=8, so most signed arithmetic was exercised without it.

Scoped rather than applied everywhere. Added to the families where it is the
edge case: signed add/sub, saturating, averaging, signed multiply and
multiply-accumulate, divide/remainder, min/max, signed compare, arithmetic shift
and rotate, widening signed, signed reductions, and integer-to-float conversion.
118 configs, 545 rows.

Left alone elsewhere. In loads, stores, mask logical operations, permutes and
bitwise operations the column is an opaque bit pattern, and INT_MIN carries no
special meaning there. Float-to-integer conversions are also left alone: their
operand is a float, where 0x8000..0 is -0.0. That is a genuine edge case and
worth its own row, but it is a different one.

The multiply forms additionally get INT_MIN x INT_MIN, whose exact product needs
2*SEW bits and which distinguishes the high-half result from the low.

Row width follows the block. The multiply-accumulate forms carry three columns
because vd is also an input.
Explains why each added value is there. The configs carry no comments today, so
this is a departure from house style -- it is a separate commit for that reason
and can be dropped without touching the vectors themselves.

The reasoning is the part a hex list cannot carry. 0x7fffffffffffffff at imm=31
looks arbitrary next to 0x0 and -1; what matters is that the first separates a
shift by 31 from one by 63 and the other two do not. Without that written down
the value reads as redundant and is a natural candidate for a later tidy-up.

Density follows information, not line count. The shift, rotate, saturation and
divide additions get a comment per group, because 33, 48 and 63 are there for
different reasons than 15 and 16. The INT_MIN rows get one comment per file:
they are ~545 rows expressing one idea, and repeating it per row would be noise
in review and 545 copies to update if the reasoning changes. 168 comments for
roughly 600 rows.

Verified the decoder accepts comments inside the arrays, and that generation is
byte-identical with them present -- 1,598 stage-1 files either way.
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.

1 participant