Conversation
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
alanhc
force-pushed
the
rvv-widen-overlap-fix
branch
from
September 10, 2026 23:18
83aad0d to
d787a91
Compare
rvv_cross_eew_overlap_illegal() applies one condition to both the
widening and the narrowing direction, and says so:
Both widening and narrowing share this predicate because the spec rule
is symmetric: overlap is allowed only when the lower-numbered half of
the wider group is the narrower group itself (reg_a == reg_b).
V 1.0 §5.2 states two conditions rather than one. A destination group may
overlap a source group when the destination EEW is smaller and the
overlap is in the lowest-numbered part of the source group, or when the
destination EEW is greater, the source EMUL is at least 1, and the
overlap is in the highest-numbered part of the destination group.
Narrowing takes the destination as the narrower group, so "lowest part of
the source" does collapse to a shared base register and the predicate
holds. Widening is the wrong end of the wrong group: a shared base
register is accepted where the spec reserves it, and an overlap at the
top of the destination group is rejected where the spec allows it. At
SEW=16/LMUL=1 the destination group of vwadd.vv v0 is {v0,v1}, so
"vwadd.vv v0, v1, v1" is legal and raised an illegal instruction, while
"vwadd.vv v2, v2, v2" is reserved and executed.
Split the predicate in two, keeping the existing test for narrowing and
adding the top-of-destination test for widening, and move the vw* integer
ops, the vwadd/vwsub .w forms and the RVV_FP64_WIDEN_* macros onto the
latter.
The source EMUL cannot be read back from the register span, since
rvv_eew_reg_span() clamps a fractional EMUL up to a span of 1 and leaves
EMUL=1 and EMUL=1/2 indistinguishable, so it comes from rvv_lmul_ratio()
instead. Segment indexed loads are not a widening site at all: the
destination EEW is SEW and the source EEW is the index EEW, with neither
fixed relative to the other, so the direction is selected from the two
EEWs. The spans cannot stand in for that comparison, since a fractional
EMUL clamps to a span of one register and leaves two different EEWs
looking identical.
tests/rvv-smoke.S covers the legal direction, which fails without this
change. The reserved cases stay uncovered: an illegal instruction in user
mode reaches __trap_handler and the run still exits 0, which is the
scaffolding issue sysprog21#735 asks for.
alanhc
force-pushed
the
rvv-widen-overlap-fix
branch
from
September 10, 2026 23:18
d787a91 to
9cfd5c0
Compare
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.
Fixes the cross-EEW overlap bug I mentioned in #734.
rvv_cross_eew_overlap_illegal()applies one condition to both the wideningand the narrowing direction, and its comment says the spec rule is symmetric.
V 1.0 §5.2 gives two different conditions. A destination group may overlap a
source group when the destination EEW is smaller and the overlap is in the
lowest-numbered part of the source group, or when the destination EEW is
greater, the source EMUL is at least 1, and the overlap is in the
highest-numbered part of the destination group.
Narrowing takes the destination as the narrower group, so "lowest part of the
source" collapses to the shared-base-register test the helper implements and
is correct there. Widening is the wrong end of the wrong group: a shared base
register is accepted where the spec reserves it, and an overlap at the top of
the destination group is rejected where the spec allows it.
At SEW=16/LMUL=1 the destination group of
vwadd.vv v0is {v0,v1}, sovwadd.vv v0, v1, v1is spec-legal and raised an illegal instruction, whilethe reserved
vwadd.vv v2, v2, v2was accepted. The spec's own wideningexample,
vzext.vf4 v0, v6at LMUL=8, is inverted the same way.The predicate is split in two, keeping the existing test for narrowing and
adding the top-of-destination test for widening, and the
vw*integer ops,the
vwadd/vwsub.wforms and theRVV_FP64_WIDEN_*macros move ontothe latter.
Two things worth flagging for review:
Source EMUL cannot be read back from the register span, since
rvv_eew_reg_span()clamps a fractional EMUL up to a span of one registerand leaves EMUL=1 and EMUL=1/2 indistinguishable, so it comes from
rvv_lmul_ratio()instead. This is the same reasoning as in #767.Segment indexed loads are not a widening site at all. The destination EEW is
SEW and the source EEW is the index EEW, with neither fixed relative to the
other, so they pick the direction by comparing the two spans and keep the
plain overlap check when the EEWs agree.
tests/rvv-smoke.Scovers the legal direction, which fails without thischange. The reserved cases stay uncovered: an illegal instruction in user
mode reaches
__trap_handlerand the run still exits 0, which is thescaffolding #735 item 1 asks for.
make checkpasses withEXT_Venabled; clang-format 20.1.7 clean.Note on ordering: #771 carries a private copy of this rule under the same
name, added because the shared helper was wrong. If this lands first, that
copy should be dropped in favour of the shared predicate when #771 rebases —
git merges the two without reporting a conflict, but the result has two
definitions of
rvv_widen_overlap_illegal()and does not compile.Summary by cubic
Fixes the cross-EEW overlap check for widening vector ops so it matches V 1.0 §5.2. Widening and narrowing now use separate predicates; the old shared rule wrongly accepted reserved overlaps and rejected legal ones. For example,
vwadd.vv v0, v1, v1at SEW=16/LMUL=1 now executes, whilevwadd.vv v2, v2, v2now traps.Details
rvv_cross_eew_overlap_illegal()intorvv_narrow_overlap_illegal()(unchanged behavior) andrvv_widen_overlap_illegal(), which requires source EMUL >= 1 and overlap at the highest-numbered destination part.rvv_lmul_ratio()becauservv_eew_reg_span()cannot distinguish EMUL=1 from fractional EMUL.#771carries a private copy of this helper; when it rebases, drop that copy to avoid duplicatervv_widen_overlap_illegal()definitions.Written for commit 9cfd5c0. Summary will update on new commits.