Repository navigation
[risc-v][vector] Restore vstart after vector configuration - #11861
PiyushPatle26 wants to merge 4 commits into
Conversation
RESTORE_VECTOR writes vstart before vsetvl, which resets the saved restart index to zero. Restore vstart after the final vector configuration instruction. Add a QEMU virt64 regression using the actual save/restore macros. Check zero and nonzero vstart, vl, vtype, vcsr, all vector registers, and frame guards. Validation: the original macro fails for nonzero vstart at VLEN=128 and VLEN=256. The fix passes the isolated probe and the RT-Thread shell regression at both VLENs. Strict BSP builds pass with vector support enabled and disabled; the disabled image boots with QEMU vector support off.
|
|
|
👋 感谢您对 RT-Thread 的贡献！Thank you for your contribution to RT-Thread! 为确保代码符合 RT-Thread 的编码规范，请在你的仓库中执行以下步骤运行代码格式化工作流（如果格式化CI运行失败）。 🛠 操作步骤 | Steps
完成后，提交将自动更新至 如有问题欢迎联系我们，再次感谢您的贡献！💐 |
📌 Code Review Assignment🏷️ Tag: libcpu_riscvReviewers: @Yaochenger Changed Files (Click to expand)
📊 Current Review Status (Last Updated: 2026-10-12 18:35 CST)
📝 Review Instructions
|
|
@Rbb666 @Yaochenger can you guys review the PR |
|
@GuEe-GUI 大佬帮忙看看？ |
There was a problem hiding this comment.
🟡 Changes recommended
The assembly probe uses an invalid immediate operand with csrrc, preventing vector-enabled builds.
2 open findings
What changed in this PR
Corrects RVV context restoration so nonzero vstart survives vsetvl, with QEMU virt64 regression coverage. / 修复 RVV 上下文恢复顺序并添加回归测试。
Changes:
- Restore
vstartafter vector configuration. - Add complete vector-frame and guard validation.
- Conditionally build the assembly probe.
| File | Description |
|---|---|
libcpu/risc-v/vector/rvv-1.0/rvv_context.h |
Reorders vstart restoration. |
bsp/qemu-virt64-riscv/applications/test/SConscript |
Adds conditional probe compilation. |
bsp/qemu-virt64-riscv/applications/test/rvv_vstart_tc.c |
Adds RVV regression tests. |
bsp/qemu-virt64-riscv/applications/test/rvv_vstart_probe_gcc.S |
Exercises save/restore macros. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| .type rvv_vstart_probe, @function | ||
| rvv_vstart_probe: | ||
| /* Keep interrupts masked until the caller's complete state is restored. */ | ||
| csrrc a3, sstatus, 2 |
| LOAD t3, VEC_FRAME_VL(t1) | ||
| VEC_CONFIG_SET_VL_VTYPE(t3, t0) | ||
|
|
||
| /* vsetvl resets vstart, so restore the restart index last. */ |
|
The title is suggested to be changed to: [risc-v][vector] Restore vstart after vector configuration. |
|
Reviewed commit English: Moving the 中文：将
English: This review is based on source inspection, caller-path checks, specifications, and CI logs. I have not independently rerun the QEMU regression. 中文：本次审核基于源码、调用路径、规范和 CI 日志，未独立复跑 QEMU 回归测试。 |
|
PiyushPatle26
changed the title
Address review feedback on the restore regression. Give each invocation its own heap-backed frames and release them on both success and comparison failure. Require heap support for the test. Use the explicit csrrci instruction and protect assembler macro bodies from C formatting. Keep normal preprocessor declarations formatted. Reproduced the nonzero-vstart failure with the original macro and verified the revised test in the full QEMU BSP at VLEN=128 and VLEN=256. The extension-disabled strict build and boot also pass. clang-format checks pass with versions 18 and 23.
|
Thanks @Rbb666 for the review. I pushed the corrections in
I reran the revised regression in the actual
The original-macro runs used the revised test and probe, replacing only Environment: GCC Reproduction commands and temporary toolchain settingsThe installed bare-metal GCC has no bundled libc. I used the following validation-only setup in an isolated checkout. These compiler/library changes are not part of the PR. This requires GCC 13.2, QEMU 8.2.2, Python, pip, curl, and tar already installed. git clone --branch fix/riscv-rvv-vstart https://github.com/PiyushPatle26/rt-thread.git rvv-review
cd rvv-review
git checkout 36afba2823
export RVV_REPRO_ROOT="$PWD"
python3 -m pip install --target "$RVV_REPRO_ROOT/.validation/python" scons kconfiglib
export PYTHONPATH="$RVV_REPRO_ROOT/.validation/python"
export PKGS_DIR="$RVV_REPRO_ROOT/.validation/packages"
export RTT_EXEC_PATH=/usr/bin
mkdir -p "$PKGS_DIR" "$RVV_REPRO_ROOT/.validation/toolchain"
curl -fL https://github.com/RT-Thread/toolchains-ci/releases/download/v1.4/riscv64-unknown-elf-toolchain-10.2.0-2020.12.8-x86_64-linux-ubuntu14.tar.gz |
tar -xz -C "$RVV_REPRO_ROOT/.validation/toolchain" --wildcards '*/riscv64-unknown-elf/include/*' '*/riscv64-unknown-elf/lib/rv64imac/lp64/*'
python3 - <<'PYSETUP'
import os
from pathlib import Path
root = Path(os.environ['RVV_REPRO_ROOT'])
base = next((root / '.validation/toolchain').glob('*/riscv64-unknown-elf'))
p = root / 'bsp/qemu-virt64-riscv/rtconfig.py'
s = p.read_text().replace('-march=rv64imafdc', '-march=rv64imafdc_zicsr_zifencei')
s += f"\nCFLAGS += ' -isystem {base}/include'\nCXXFLAGS = CFLAGS\n"
s += f"LFLAGS += ' -L{base}/lib/rv64imac/lp64'\n"
s += "AFLAGS = AFLAGS.replace('rv64imafdc_zicsr_zifencei', 'rv64imafdcv_zicsr_zifencei')\n"
s += f"DEVICE += ' -isystem {base}/include'\n"
p.write_text(s)
PYSETUPFor each VLEN, edit # Set RVV_VLEN=128 first, then repeat with 256.
export RVV_VLEN=128
python3 - <<'PYCONFIG'
import os
from pathlib import Path
p = Path('bsp/qemu-virt64-riscv/.config')
s = p.read_text().replace('# CONFIG_ENABLE_VECTOR is not set', 'CONFIG_ENABLE_VECTOR=y')
for n in (128, 256):
key = f'CONFIG_ARCH_VECTOR_VLEN_{n}'
enabled = os.environ['RVV_VLEN'] == str(n)
s = s.replace(key + '=y', '# ' + key + ' is not set')
if enabled:
s = s.replace('# ' + key + ' is not set', key + '=y')
p.write_text(s)
PYCONFIG
python3 -m SCons --pyconfig-silent -C bsp/qemu-virt64-riscv
python3 -m SCons -j4 --strict -C bsp/qemu-virt64-riscv
qemu-system-riscv64 -machine virt -cpu "rv64,v=true,vlen=$RVV_VLEN,vext_spec=v1.0" -smp 1 -m 256M -nographic -kernel bsp/qemu-virt64-riscv/rtthread.binAt the Captured shell output: For the other CI failures, the Tools job ends with Also, could you please share guidance on the open scope and extension-use-policy questions in #11786? I would like to agree on the next FP correctness work before proposing lazy switching. The initial scope remains single-hart RISC-V. Update: the fork BSP build for |
There was a problem hiding this comment.
It is recommended to move the test and its assembly probe to a dedicated utest directory and enable conditional build integration. Add additional CI configuration to enable vector support, heap, and UTest for VLEN=128 and VLEN=256.
You can refer to the BSP of K230:
There was a problem hiding this comment.
Thanks for the guidance. I moved the regression and assembly probe to applications/utest/ with conditional build integration, and added CI configurations for VLEN=128 and 256.
Both vector-enabled builds and the new UTest regression pass locally. I also added a separate minimal commit fixing the legacy testcase’s compilation errors. Please review the updated commits.
Move the regression and assembly probe into a dedicated UTest directory. Build them only with Vector, heap, and UTest enabled, and use the UTest entry point. Add strict CI attachconfig variants for VLEN=128 and VLEN=256. Enable V in assembler flags only for vector builds. Both regression objects compile at both VLENs. The vector-disabled full UTest build passes. Vector-enabled full builds expose pre-existing failures in the legacy testcase, addressed separately.
Use sizeof(rt_ubase_t) for the vector frame size instead of the assembly-only REGBYTES macro. Provide test-only wrappers for the missing save/restore helpers, and enable vector instructions only for the legacy testcase compilation. Keep the legacy testcase flow unchanged. Strict Vector + UTest builds pass at VLEN=128 and VLEN=256, and the new vstart regression passes through UTest at both VLENs. The vector-disabled full build passes with vector test symbols excluded. The legacy runtime testcase itself was not executed.
拉取/合并请求描述：(PR description)
为什么提交这份PR (why to submit this PR)
RESTORE_VECTORrestoresvstartbeforevsetvl, which resets the saved restart index to zero. Restore it after the final vector configuration instruction.你的解决方案是什么 (what is your solution)
Move the
vstartrestore to the end ofRESTORE_VECTOR, without changing the frame layout. Add a QEMU virt64 regression using the actual save/restore macros to check zero and nonzerovstart,vl,vtype,vcsr, all vector registers, and frame guards.The regression and its probe live in
applications/utest/, with conditional SCons integration and CI attachconfig variants for both VLENs. Each invocation owns its heap-backed frames, including its caller-state backup, so msh and UTest calls do not share mutable buffers. The test requires heap support. The probe uses explicitcsrrci; assembler macro bodies are protected from C formatting.请提供验证的bsp和config (provide the config and bsp)
bsp/qemu-virt64-riscv.config:CONFIG_ENABLE_VECTOR=y,CONFIG_RT_USING_HEAP=y,CONFIG_FINSH_USING_MSH=y, SMP disabled,CONFIG_RT_USING_UTEST=y. TestCONFIG_ARCH_VECTOR_VLEN_128=yandCONFIG_ARCH_VECTOR_VLEN_256=yseparately; runutest_run testcases.libcpu.rvv_vstart.v=false; the test is excluded. Concurrent msh/UTest execution and SMP have not been runtime-tested.当前拉取/合并请求的状态 Intent for your PR
必须选择一项 Choose one (Mandatory):
代码质量 Code Quality：
我在这个拉取/合并请求中已经考虑了 As part of this pull request, I've considered the following:
#if 0代码，不包含已经被注释了的代码 All redundant code is removed and cleaned upLatest validation: full Vector + heap + UTest builds and the focused UTest regression pass at both VLENs. The legacy testcase received a separate compilation fix; its runtime flow was not changed or executed.