Repository navigation
Conversation
Two bugs make i?min return a wrong index on RISCV64_ZVL256B (imin_vector.c with the v1.0 RVV intrinsics): - The v1.0 port (2406958) maps the masked vid/vadd to the mask-agnostic __riscv_vid_v_*_m / __riscv_vadd_vx_*_m forms and drops the merge operand, so the indices of lanes whose minimum was found in an earlier strip are lost. Use the mask-undisturbed _mu forms with v_min_index as the merge operand, as imax_vector.c does and as the RISCV_0p10_INTRINSICS branch already does. - The tail compares cur_minf > minf, which can never hold because cur_minf is reduced from minf; a minimum in the tail was ignored. Compare with <, as iamin_vector.c does.
This branch has not been deployed
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.
Summary
On RISCV64_ZVL256B,
cblas_ismin/cblas_idmin(and ISMIN/IDMIN) return a wrongindex for many inputs longer than one vector strip.
imin_vector.chas two bugsin its RVV 1.0 intrinsics branch:
The merge operand is missing. The masked
vid/vaddinstructions thatrecord each lane's running-minimum index use the mask-agnostic
__riscv_vid_v_*_m/__riscv_vadd_vx_*_mforms, which take no mergeoperand. A lane whose minimum was found in an earlier strip therefore loses
its index.
imax_vector.cgot the mask-undisturbed_muforms.RISCV_0p10_INTRINSICSbranch ofimin_vector.cstill passesv_min_indexas the merge operand.The patch uses the
_muforms, withv_min_indexas the merge operand.The tail comparison is inverted. The tail checks
cur_minf > minf. Thatcan never be true, because
cur_minfis reduced fromminf, so a minimum inthe last partial strip is ignored. The check is now
cur_minf < minf, as iniamin_vector.c.Both the unit-stride and the strided paths are fixed. Not affected:
imin_rvv.c.Testing
Platform: SpacemiT K3, one X100 core,
TARGET=RISCV64_ZVL256B, GCC 15.2.Targeted check of
i?amax,i?amin,i?max,i?min(S and D):position, and as a tie;
Results:
ISMIN with n = 65 returns 6 instead of 64.
OpenBLAS tests. The default
makegoal's tests give identical resultsbefore and after the patch: utest 141/141, extension tests 1479/1479, and all
BLAS/CBLAS tests PASSED.
Performance. The patched kernel runs exactly as fast as its sibling
imax_vector.c. At n = 1024, ISMIN/IDMIN take 472/821 ns per call, andISMAX/IDMAX take 473/821 ns. The old kernel was faster on in-cache sizes
(257/387 ns) only because dropping the merge operand also removed the
loop-carried dependency on the index vector, at the cost of losing the
indices. At n = 4M the patched kernel is about 3 % faster.
This bug was found while comparing the
_vectorand_rvvlevel-1 kernels onthe X100.
Disclosure: an AI coding agent (Anthropic Claude) prepared this patch, its
validation runs and this description on behalf of the account owner. All
numbers above come from archived runs on the board.