Re: [RFC PATCH 1/5] LoongArch: Fix some instruction formats

[Youling Tang <tangyouling@xxxxxxxxxxx>]

Hi, Tiezhu

On 08/09/2022 10:52 AM, Tiezhu Yang wrote:
> struct reg2i12_format is used to generate the instruction lu52id
> in larch_insn_gen_lu52id(), according to the instruction format
> of lu52id in LoongArch Reference Manual [1], the type of field
> "immediate" should be "signed int" rather than "unsigned int".
>
> There are similar problems in the other structs reg0i26_format,
> reg1i20_format, reg1i21_format and reg2i16_format, fix them.
>
> [1] https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#_lu12i_w_lu32i_d_lu52i_d
>
> Fixes: b738c106f735 ("LoongArch: Add other common headers")
>
We may not be able to say "Fixes" here, because it is also correct to
treat each field of the instruction as an "unsinged int" type (signed
or not has no effect on the machine instruction stream, but it does
affect the programmer).

For example, when reg2i12_format.immediate is changed to "signed" type,
the immediate judgment in is_stack_alloc_ins() can be simplified,

static inline bool is_stack_alloc_ins(union loongarch_instruction *ip)
{
    /* addi.d $sp, $sp, -imm */
    return ip->reg2i12_format.opcode == addid_op &&
        ip->reg2i12_format.rj == LOONGARCH_GPR_SP &&
        ip->reg2i12_format.rd == LOONGARCH_GPR_SP &&
-        is_imm12_negative(ip->reg2i12_format.immediate);
+        (ip->reg2i12_format.immediate < 0;
}


Thanks,
Youling
> Signed-off-by: Tiezhu Yang <yangtiezhu@xxxxxxxxxxx>
> ---
>  arch/loongarch/include/asm/inst.h | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/arch/loongarch/include/asm/inst.h b/arch/loongarch/include/asm/inst.h
> index 7b07cbb..ff51481 100644
> --- a/arch/loongarch/include/asm/inst.h
> +++ b/arch/loongarch/include/asm/inst.h
> @@ -53,35 +53,35 @@ enum reg2i16_op {
>  };
>
>  struct reg0i26_format {
> -	unsigned int immediate_h : 10;
> -	unsigned int immediate_l : 16;
> +	signed int immediate_h : 10;
> +	signed int immediate_l : 16;
>  	unsigned int opcode : 6;
>  };
>
>  struct reg1i20_format {
>  	unsigned int rd : 5;
> -	unsigned int immediate : 20;
> +	signed int immediate : 20;
>  	unsigned int opcode : 7;
>  };
>
>  struct reg1i21_format {
> -	unsigned int immediate_h  : 5;
> +	signed int immediate_h  : 5;
>  	unsigned int rj : 5;
> -	unsigned int immediate_l : 16;
> +	signed int immediate_l : 16;
>  	unsigned int opcode : 6;
>  };
>
>  struct reg2i12_format {
>  	unsigned int rd : 5;
>  	unsigned int rj : 5;
> -	unsigned int immediate : 12;
> +	signed int immediate : 12;
>  	unsigned int opcode : 10;
>  };
>
>  struct reg2i16_format {
>  	unsigned int rd : 5;
>  	unsigned int rj : 5;
> -	unsigned int immediate : 16;
> +	signed int immediate : 16;
>  	unsigned int opcode : 6;
>  };