Value class coverage - #97
Open
cwssemi wants to merge 3 commits into
Open
Conversation
…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.
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.
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:
extend value coverage for shifts, rotates, saturation and divide—vror.viatuimm >= 32(its immediate is six bits, the top onefunct6[0], so that encoding wasunreached); the two ends of the
simm5range; operand pairs that reach the saturationboundary where none did;
INT_MIN / -1; and an operand that separates a shift by 31from one by 63 where
0x0and-1cannot.add INT_MIN operands to the signed-arithmetic families— 118 configs. Scoped towhere
INT_MINis the edge case, and deliberately not added to loads, stores, masklogic or permutes, where the column is an opaque bit pattern.
comment the added value classes— 191 comments across 128 configs explaining why each value isthere. 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 theywere 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.viatimm >= 32shifts arithmetically. The rotate's shift half sign-extends,because the signedness flag is taken from
funct6(0)— which means arithmetic for the shiftfamily but direction for the rotate family. Reached only by the
uimm >= 32values incommit 1; the largest immediate the configs declared before was 31, so
object RORIhad neverbeen executed by a generated test at any SEW.
vsmulsaturates at 32-bit bounds when SEW=64. A ScalaIntoverflow in a Chiselconstant:
1 << ((8 << sew)-1)masks the shift distance to 5 bits, sosew=3clips at ±2^31.A genuine saturation returns
INT32_MAXinstead ofINT64_MAX, and a result merely largerthan
INT32_MAXis clipped although the spec does not saturate it, raisingvxsatspuriously.Reached only by the
INT_MINoperands in commit 2.Neither is exotic.
INT_MIN × INT_MINis the single defined saturation case forvsmul, andvror.vihas a six-bit immediate of which the configs exercised five bits. Both are the kind ofvalue 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.ymldoesmake generate-stage1thenmake allacross VLEN 256/128/64 × XLEN 64/32. 128 configs is a widechange 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
generate-stage1passes: 1,612 files, against 1,555 before. Byte-identicalwith and without the comment commit.
base/sew8/sew16/sew32, quoted strings forsew64, matching theuint8/16/32andstringfields thedecoder expects.
vdis also an input.Related
Separate from #94 and its PR, which
is a generator change restoring the declared
sew64blocks for thevd,vs2,uimm,vmandvd,vs2,vmformats. 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.