Skip to content

[hardware] 🐛 Fix two vfirst.m defects in the mask unit (#500) - #502

Open
renzoandri wants to merge 2 commits into
pulp-platform:mainfrom
renzoandri:fix/masku-vfirst
Open

renzoandri wants to merge 2 commits into
pulp-platform:mainfrom
renzoandri:fix/masku-vfirst

Conversation

@renzoandri

Copy link
Copy Markdown

Two independent vfirst.m defects in masku.sv, one commit each, smallest
first. Details and reproducers in #500.

  • 1/2 — index accumulation. The accumulator added lzc's cnt_o
    unconditionally, but common_cells' lzc returns WIDTH-1 (not WIDTH) for
    an all-zero input, so vfirst.m under-reported by exactly
    floor(idx / VfirstParallelism). Advance by VfirstParallelism on an empty
    slice.
  • 2/2 — stranded operands. vfirst.m terminated the instruction on the first
    set bit, leaving the operand words the lanes were still delivering for the rest
    of that vl in the per-lane MASKU operand queue, where the next mask
    instruction consumed them. Same class as masku_operands: drain ALU data during VID #448 — whose vid.v drain is not on
    main either; it arrives with 🐛 [masku] Mask-unit correctness & deadlock fixes (#446 #448 #450) #469's 69946aa. vfirst.m now runs to
    completion like vcpop.m — consuming every slice is the drain — with a
    vfirst_found_q flag so the extra slices cannot disturb the latched answer.

Cost of 2/2: vfirst.m always takes ceil(vl/VfirstParallelism) MASKU cycles,
the same as vcpop.m already does. Keeping the early exit would need a real
drain counter for the operands still in flight.

Relation to #469 — no conflict, and #469 is the better base. The two series
touch different code: #469 touches vfirst.m only at the vcpop_operand
assignment (#446's VL trim), while these commits touch the index accumulator, the
two operand-ack conditions and the out_scalar_valid condition. Verified rather
than assumed — git am -3 of this series onto #469's head (aee901c7)
auto-merges masku.sv with no conflict; only the CHANGELOG.md ### Fixed list
collides, trivially.

Stacking on #469 is also the tested configuration: the tree these fixes were
characterised on already carried equivalents of #469's two mask-unit hunks (the
#446 VL trim and the #448 vid.v drain — see the issue for the mapping), so
main + #469 + this series is what was simulated, and this series alone on
main is the untested arrangement. Both apply cleanly either way — say the word
and I'll rebase on #469, or on main first if you'd rather take these
independently.

Not #494. That open issue also names vfirst.m, but it is the missing
vstart != 0 illegal-instruction check the RVV 1.0 mandatory list requires, and
it is fixed at the decode site in ara_dispatcher.sv. This series does not touch
that file — only masku.sv and CHANGELOG.md — so the two are independent.

Changelog

Fixed

  • Fix vfirst.m index under-reporting by floor(idx/VfirstParallelism) on
    all-zero mask slices
  • Run vfirst.m to completion so it drains its MASKU operands, instead of
    aborting on the first set bit

Checklist

  • Automated tests pass — see Verification below
  • Changelog updated
  • Code style guideline is observed

Verification

ara_tb_verilator, NR_LANES=2, VLEN=2048, verilator 5.044, on the pin
ab4158ae (whose masku.sv is byte-identical to main at 34bd3bc) with the
ianfield fork's #446/#448/#451 fixes applied — two of which are #469's mask-unit
hunks, as above. These two commits have not been simulated on plain main
without those, which is the other reason #469 is the base I'd suggest:

  • A directed reproducer suite — 16 ELFs, 33 probes, covering the 2×2×2 factorial
    over {fault-only-first | plain load} × {mask dest overlaps load dest |
    separate} × {csrr vl | none}, plus a 10-point index sweep — goes from
    FAIL/HANG to PASS on every phase, including GCC's own compiled strlen idiom.
    The index sweep (set bit at 0, 1, 15, 16, 17, 31, 32, 33, 100, 255) is exact at
    every point.
  • The mask-unit regression suite (11 variants) stays green.
  • No perturbation of anything already measured: a hello_world run is
    bit-identical at 7625 cycles / 3751 instret / IPC 0.491, and an
    attention-kernel benchmark reproduces figure-for-figure (fp32 782007 scalar /
    71512 vector / 10.94×, int8 94067, max|Δy| 9.28e-4, both bit-exact).

Why the in-tree suite is green today, since that is the first thing to check:
apps/riscv-tests/isa/rv64uv/vfirst.c is the only test that executes
vfirst.m, and both its cases run at vl = 4 — one VfirstParallelism = 16
slice (a fixed localparam, not a function of NrLanes), which reaches neither
defect. Its all-zero-mask -1 expectation is the one commit 2/2 touches, and it
is preserved by construction; the CI run on this PR is what exercises it.

Note the patch adds one flop and changes MASKU control, so it has not been
re-synthesised here; the numbers above are all RTL simulation.

Provenance

Prepared with AI assistance — LLM agents did the defect analysis, drafted the two
patches, and ran the simulation campaign above. I have reviewed the series and the
numbers before sending it. Nothing here rests on taking that on trust: the
reproducers, the exact commands and the pin are all in the linked issue, so every
claim is checkable. Tell me if you would rather any part of it were reworked or
rewritten.

The MASKU index accumulator for `vfirst.m` added the leading-zero count of
every slice unconditionally:

    vfirst_count_d = vfirst_count_q + vfirst_count;

`common_cells`' `lzc` documents that for an all-zero input it asserts
`empty_o` and leaves `cnt_o` at "the maximum number of zeros - 1" -- i.e.
`VfirstParallelism - 1`, not `VfirstParallelism`. Every fully-masked slice
therefore advanced the index by one less than the number of elements it
covered, and `vfirst.m` under-reported the result by exactly
`floor(idx / VfirstParallelism)`.

Observed with `VfirstParallelism = 16` (NR_LANES=2, VLEN=2048), sweeping the
position of the single set bit:

    idx  16 -> 15    idx  31 -> 30    idx  33 -> 31
    idx  17 -> 16    idx  32 -> 30    idx 100 -> 94    idx 255 -> 240

Positions below `VfirstParallelism` are unaffected, which is why the bug is
invisible to short vectors -- including GCC's inline RVV `strlen` expansion on
strings shorter than one slice.

Advance the index by `VfirstParallelism` when the slice is empty.
`vfirst.m` terminated the instruction as soon as the first set bit was found,
through three `|| (!vfirst_empty && op == VFIRST)` terms: the two operand-ack
sites and the `out_scalar_valid` site.

The lanes, however, keep delivering the source-mask operand words for the rest
of that `vl`. With the instruction already retired nobody acknowledged them, so
they stayed in the per-lane MASKU operand queue and the *next* mask instruction
consumed them as its own input. The symptom is that the first mask sequence a
program runs is correct and every later one is wrong -- typically 0 -- and with
enough accumulated residue the mask unit deadlocks.

This is the same class of defect as pulp-platform#448. (pulp-platform#448's `vid.v` drain is not on `main`
either: `masku_operands.sv` at 34bd3bc has no VID case; it arrives with PR pulp-platform#469's
69946aa.)

Drop the three early-termination terms so `vfirst.m` runs to completion exactly
like `vcpop.m`, which has always consumed every slice and has never shown this
failure. Consuming every slice *is* the drain. A new `vfirst_found_q` flag
carries "the answer is already latched" so the extra slices cannot disturb the
result, and the "no set bit in vl" test becomes `!vfirst_found_d` rather than
"the slice we stopped on was empty" -- with the early exit gone, the last slice
of a *successful* `vfirst.m` is normally empty, so the old test would have
returned -1.

Cost: `vfirst.m` now always takes ceil(vl/VfirstParallelism) MASKU cycles, the
same as `vcpop.m`. Keeping the early exit would need a real drain counter for
the operands still in flight.

Reproducer: GCC 15's inline RVV `strlen` expansion for rv64gcv
(`vle8ff.v` + `vmseq.vi` + `csrr vl` + `vfirst.m`) in a loop. On ara_tb_verilator
with NR_LANES=2, VLEN=2048 the first call returns the right length and every
later call returns 0.

This branch has not been deployed

No deployments
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