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

[Nam Cao <namcao@linutronix.de>]

Charlie Jenkins via B4 Relay
<devnull+thecharlesjenkins.gmail.com@kernel.org> writes:
> From: Charlie Jenkins <thecharlesjenkins@gmail.com>
>
> Eliminate the need to hand-write riscv instructions by using a shell
> script to autogenerate a header from an instruction table. This is modeled
> after the syscall table infrastructure.
>
> The table is generated externally by riscv-unified-db [1], but is
> in a simple format to make it possible to use other tools or modify
> manually.
>
> [1] https://github.com/riscv-software-src/riscv-unified-db
>
> Signed-off-by: Charlie Jenkins <thecharlesjenkins@gmail.com>

Thanks for the work, I really like the idea. This will make it much
easier to maintain the instruction stuffs.

> +c.ld common,32 011<13|00<0 imm<3=6-5|12-10 xd!1!3!5!7=4-2 xs1=9-7
> +c.ld common,64 011<13|00<0 imm<3=6-5|12-10 xd=4-2 xs1=9-7

Not sure if I confuse something, but the spec says "C.LD is an
RV64C-only instruction". Why do we have 32 here?

> +echo "#define COMMA ," >> $outfile
> +echo "#define SEMICOLON ;" >> $outfile
> +echo "#define SINGLE_ARG(...) __VA_ARGS__" >> $outfile

Aren't these macro unused?

> +echo >> $outfile
> +
> +grep -E "^[a-z\.0-9]+[[:space:]]+" "$infile" | {
> +    while read name base fixed variables; do
> +        echo "/* $name */"
> +
> +        compressed_name=${name##c.*}
	   ^^^^^^^^^^^^^^^
	   this name is misleading

> +        invalid_inst_functions=""
> +        variable_params=""
> +        constraints=""
> +        match=""
> +        mask=""
> +        make=""
> +
> +        # All compressed instructions start with "c."
> +        size=${compressed_name:+32};
> +        size=${size:-16};
> +
> +        # Replace all . with _
> +        formatted_inst_name=$name
> +        while [ ! ${formatted_inst_name##*.*} ]; do
> +            prefix=${formatted_inst_name%.*}
> +            suffix=${formatted_inst_name##*.}
> +            contains_dot=${formatted_inst_name##*.*}
> +            formatted_inst_name=${contains_dot:-${prefix}_${suffix}}
> +        done

Does the simplier
     formatted_inst_name=$(echo $name | tr '.' '_')
work?

> +            echo "static __always_inline ${type}${size} riscv_insn_${formatted_inst_name}_extract_${variable_name}(u${size} ${insn})"
> +            echo "{"
> +            echo "\treturn ${extract};"
> +            echo "}"
> +            echo "static __always_inline void riscv_insn_${formatted_inst_name}_insert_${variable_name}(u${size} *${insn}, ${type}32 ${var})"
> +            echo "{"
> +            echo "\t*_insn &= ${insert_mask# & };"

Why is this required? Isn't this part always zero at this point?

> +            echo "\t*_insn |= ${insert# | };"
> +            echo "}"
> +
> +            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\tpanic(\"${name} is not supported on non ${only_base}-bit systems.\");\n}\n"

Instead of panic(), can we do BUILD_BUG() instead?

Nam