Re: [PATCH v3 01/17] riscv: Introduce instruction table generation

[Charlie Jenkins <thecharlesjenkins@gmail.com>]

On Wed, Jul 08, 2026 at 03:50:47AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] Zcmp compressed instructions (e.g., `cm.*`) are incorrectly generated with 32-bit sizes instead of 16-bit.
> - [Low] Unsupported instruction extraction stubs lack a return statement, causing `-Wreturn-type` build errors.
> --
> 
> --- Patch [1]: [PATCH v3 01/17] riscv: Introduce instruction table generation ---
> commit 893d119e65d6edd9ce9e98809ffad3450c8d6a8f
> Author: Charlie Jenkins <thecharlesjenkins@gmail.com>
> 
> riscv: Introduce instruction table generation
> 
> This commit introduces a shell script to auto-generate a header from an
> instruction table for RISC-V. It aims to replace hand-written instruction
> macros.
> 
> Link: https://github.com/riscv-software-src/riscv-unified-db
> 
> > diff --git a/arch/riscv/tools/insn_tbl.sh b/arch/riscv/tools/insn_tbl.sh
> > new file mode 100755
> > index 0000000000000..f09b93951cde5
> > --- /dev/null
> > +++ b/arch/riscv/tools/insn_tbl.sh
> > @@ -0,0 +1,258 @@
> 
> [ ... ]
> 
> > +        non_compressed_insn=${name##c.*}
> > +        invalid_inst_functions=""
> > +        variable_params=""
> > +        constraints=""
> > +        match=""
> > +        mask=""
> > +        make=""
> > +
> > +        # All compressed instructions start with "c."
> > +        size=${non_compressed_insn:+32};
> > +        size=${size:-16};
> 
> [Severity: Medium]
> Will this correctly identify Zcmp compressed instructions like cm.pop?
> 
> Since they start with "cm." instead of "c.", the glob pattern c.* won't
> match them, assigning them a 32-bit size instead of a 16-bit size. Could this
> cause the generated insert and extract helpers to wrongly expect u32 *
> instead of u16 *?

I'll make it check for "cm.*" as well. It looks like "cm.*" is the only
other possiblity but there are some names like "czero." and "cbo." that
prevent me changing this to matching on "c*.*".

> 
> [ ... ]
> 
> > +            printf "static __always_inline void riscv_insn_${formatted_inst_name}_insert_${variable_name}(u${size} *${insn}, ${type}32 ${var})\n"
> > +            printf "{\n"
> > +            printf "\t*_insn &= ${insert_mask# & };\n"
> > +            printf "\t*_insn |= ${insert# | };\n"
> > +            printf "}\n"
> > +
> > +            if [ "${only_base}" ]; then
> > +                invalid_inst_functions="${invalid_inst_functions}static __always_inline ${type}${size} riscv_insn_${formatted_inst_name}_extract_${variable_name}(u${size} ${insn}) {\n\tBUILD_BUG_ON_MSG(1, \"${name} is not supported on non ${only_base}-bit systems.\");\n}\n"
> > +            fi
> 
> [Severity: Low]
> Do these unsupported instruction extraction stubs need a return statement?
> 
> Since they have non-void return types, omitting a return could cause a build
> failure due to -Werror=return-type on supported architectures like RV32.

BUILD_BUG_ON_MSG(1, ...) makes it not cause an error when not compiling
with -O0. Adding the return will add 4kb which is not a major addition
but seems best avoided. There does seem to be a handful of other
warnings that are emitted with W=123 set though, I can fix those up.
Most of them are about some type inconsistencies between casting.

- Charlie

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260707-riscv_insn_table-v3-0-3f8e5f75ba97@gmail.com?part=1