[RFC PATCH 0/3] Fix some TCG RISC-V backend bugs

[RFC PATCH 0/3] Fix some TCG RISC-V backend bugs

[LIU Zhiwei]

This patch set fix some bugs in RISC-V backend. It includes:
1. guest regiser using bug in tcg_out_qemu_ld and tcg_out_qemu_st
2. immediate range bug in tcg_out_opc_imm
3. a wrong optimization in tcg_out_addsub2

After this patch set, I can run the 400.perlbench case(spec2006-simple). Besides,
it seems that we can also run RISU on qemu-aarch64(risc-v) now.

I try to run RISU on qemu-aarch64(x86) and qemu-aarch64(risc-v). But this caused
an segment fault. And it's confusing that RISU running on two qemu-aarch64(x86-64)
also caused a segment fault.

LIU Zhiwei (3):
  tcg/riscv: Fix base regsiter for qemu_ld/st
  tcg/riscv: Fix tcg_out_opc_imm when imm exceeds
  tcg/riscv: Remove a wrong optimization for addsub2

 tcg/riscv/tcg-target.c.inc | 35 +++++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

-- 
2.25.1

[RFC PATCH 1/3] tcg/riscv: Fix base regsiter for qemu_ld/st

[LIU Zhiwei]

When guest base is zero, we should use addr_regl as base regiser instead of
the initial register TCG_REG_TMP0.

Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
---
 tcg/riscv/tcg-target.c.inc | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tcg/riscv/tcg-target.c.inc b/tcg/riscv/tcg-target.c.inc
index 81a83e45b1..32f4bc7bfc 100644
--- a/tcg/riscv/tcg-target.c.inc
+++ b/tcg/riscv/tcg-target.c.inc
@@ -1185,6 +1185,8 @@ static void tcg_out_qemu_ld(TCGContext *s, const TCGArg *args, bool is_64)
     }
     if (guest_base != 0) {
         tcg_out_opc_reg(s, OPC_ADD, base, TCG_GUEST_BASE_REG, addr_regl);
+    } else {
+        base = addr_regl;
     }
     tcg_out_qemu_ld_direct(s, data_regl, data_regh, base, opc, is_64);
 #endif
@@ -1257,6 +1259,8 @@ static void tcg_out_qemu_st(TCGContext *s, const TCGArg *args, bool is_64)
     }
     if (guest_base != 0) {
         tcg_out_opc_reg(s, OPC_ADD, base, TCG_GUEST_BASE_REG, addr_regl);
+    } else {
+        base = addr_regl;
     }
     tcg_out_qemu_st_direct(s, data_regl, data_regh, base, opc);
 #endif
-- 
2.25.1

[RFC PATCH 2/3] tcg/riscv: Fix tcg_out_opc_imm when imm exceeds

[LIU Zhiwei]

TYPE-I immediate can only represent a signed 12-bit value. If immediate
exceed, mov it to an register.

Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
---
 tcg/riscv/tcg-target.c.inc | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/tcg/riscv/tcg-target.c.inc b/tcg/riscv/tcg-target.c.inc
index 32f4bc7bfc..bfdf2bea69 100644
--- a/tcg/riscv/tcg-target.c.inc
+++ b/tcg/riscv/tcg-target.c.inc
@@ -668,7 +668,12 @@ static void tcg_out_addsub2(TCGContext *s,
     if (!cbh) {
         tcg_out_opc_reg(s, (is_sub ? opc_sub : opc_add), th, ah, bh);
     } else if (bh != 0 || ah == rl) {
-        tcg_out_opc_imm(s, opc_addi, th, ah, (is_sub ? -bh : bh));
+        if (bh == sextract(bh, 0, 12)) {
+            tcg_out_opc_imm(s, opc_addi, th, ah, (is_sub ? -bh : bh));
+        } else {
+            tcg_out_movi(s, TCG_TYPE_TL, th, (is_sub ? -bh : bh));
+            tcg_out_opc_reg(s, opc_add, th, ah, th);
+        }
     } else {
         th = ah;
     }
@@ -676,8 +681,14 @@ static void tcg_out_addsub2(TCGContext *s,
     /* Note that tcg optimization should eliminate the bl == 0 case.  */
     if (is_sub) {
         if (cbl) {
-            tcg_out_opc_imm(s, OPC_SLTIU, TCG_REG_TMP0, al, bl);
-            tcg_out_opc_imm(s, opc_addi, rl, al, -bl);
+            if (bl == sextract(bl, 0, 12)) {
+                tcg_out_opc_imm(s, OPC_SLTIU, TCG_REG_TMP0, al, bl);
+                tcg_out_opc_imm(s, opc_addi, rl, al, -bl);
+            } else {
+                tcg_out_movi(s, TCG_TYPE_TL, rl, bl);
+                tcg_out_opc_reg(s, OPC_SLTU, TCG_REG_TMP0, al, rl);
+                tcg_out_opc_reg(s, opc_sub, rl, al, TCG_REG_TMP0);
+            }
         } else {
             tcg_out_opc_reg(s, OPC_SLTU, TCG_REG_TMP0, al, bl);
             tcg_out_opc_reg(s, opc_sub, rl, al, bl);
@@ -685,8 +696,15 @@ static void tcg_out_addsub2(TCGContext *s,
         tcg_out_opc_reg(s, opc_sub, rh, th, TCG_REG_TMP0);
     } else {
         if (cbl) {
-            tcg_out_opc_imm(s, opc_addi, rl, al, bl);
-            tcg_out_opc_imm(s, OPC_SLTIU, TCG_REG_TMP0, rl, bl);
+            if (bl == sextract(bl, 0, 12)) {
+                tcg_out_opc_imm(s, opc_addi, rl, al, bl);
+                tcg_out_opc_imm(s, OPC_SLTIU, TCG_REG_TMP0, rl, bl);
+            } else {
+                tcg_out_movi(s, TCG_TYPE_TL, TCG_REG_TMP0, bl);
+                tcg_out_opc_reg(s, opc_add, rl, al, TCG_REG_TMP0);
+                tcg_out_opc_reg(s, OPC_SLTU, TCG_REG_TMP0,
+                                rl, al);
+            }
         } else if (rl == al && rl == bl) {
             tcg_out_opc_imm(s, OPC_SLTI, TCG_REG_TMP0, al, 0);
             tcg_out_opc_reg(s, opc_addi, rl, al, bl);
-- 
2.25.1

[RFC PATCH 3/3] tcg/riscv: Remove a wrong optimization for addsub2

[LIU Zhiwei]

It's not clear what it is doing here. And it's wrong because bl and
al are both register, so we can't add them by an ADDI instruction.

Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
---
 tcg/riscv/tcg-target.c.inc | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/tcg/riscv/tcg-target.c.inc b/tcg/riscv/tcg-target.c.inc
index bfdf2bea69..a07fd0864f 100644
--- a/tcg/riscv/tcg-target.c.inc
+++ b/tcg/riscv/tcg-target.c.inc
@@ -705,9 +705,6 @@ static void tcg_out_addsub2(TCGContext *s,
                 tcg_out_opc_reg(s, OPC_SLTU, TCG_REG_TMP0,
                                 rl, al);
             }
-        } else if (rl == al && rl == bl) {
-            tcg_out_opc_imm(s, OPC_SLTI, TCG_REG_TMP0, al, 0);
-            tcg_out_opc_reg(s, opc_addi, rl, al, bl);
         } else {
             tcg_out_opc_reg(s, opc_add, rl, al, bl);
             tcg_out_opc_reg(s, OPC_SLTU, TCG_REG_TMP0,
-- 
2.25.1

Re: [RFC PATCH 1/3] tcg/riscv: Fix base regsiter for qemu_ld/st

[Richard Henderson]

On 10/20/22 20:41, LIU Zhiwei wrote:
> When guest base is zero, we should use addr_regl as base regiser instead of
> the initial register TCG_REG_TMP0.
> 
> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
> ---
>   tcg/riscv/tcg-target.c.inc | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/tcg/riscv/tcg-target.c.inc b/tcg/riscv/tcg-target.c.inc
> index 81a83e45b1..32f4bc7bfc 100644
> --- a/tcg/riscv/tcg-target.c.inc
> +++ b/tcg/riscv/tcg-target.c.inc
> @@ -1185,6 +1185,8 @@ static void tcg_out_qemu_ld(TCGContext *s, const TCGArg *args, bool is_64)
>       }
>       if (guest_base != 0) {
>           tcg_out_opc_reg(s, OPC_ADD, base, TCG_GUEST_BASE_REG, addr_regl);
> +    } else {
> +        base = addr_regl;
>       }

You're right that there's a bug here, where TMP0 remains uninitialized.  I think it would 
be better to reorg the other direction: begin with initializeing base = addr_regl, and 
then change it away only when we make modifications.


r~

Re: [RFC PATCH 2/3] tcg/riscv: Fix tcg_out_opc_imm when imm exceeds

[Richard Henderson]

On 10/20/22 20:41, LIU Zhiwei wrote:
> TYPE-I immediate can only represent a signed 12-bit value. If immediate
> exceed, mov it to an register.
> 
> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
> ---
>   tcg/riscv/tcg-target.c.inc | 28 +++++++++++++++++++++++-----
>   1 file changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/tcg/riscv/tcg-target.c.inc b/tcg/riscv/tcg-target.c.inc
> index 32f4bc7bfc..bfdf2bea69 100644
> --- a/tcg/riscv/tcg-target.c.inc
> +++ b/tcg/riscv/tcg-target.c.inc
> @@ -668,7 +668,12 @@ static void tcg_out_addsub2(TCGContext *s,
>       if (!cbh) {
>           tcg_out_opc_reg(s, (is_sub ? opc_sub : opc_add), th, ah, bh);
>       } else if (bh != 0 || ah == rl) {
> -        tcg_out_opc_imm(s, opc_addi, th, ah, (is_sub ? -bh : bh));
> +        if (bh == sextract(bh, 0, 12)) {
> +            tcg_out_opc_imm(s, opc_addi, th, ah, (is_sub ? -bh : bh));
> +        } else {
> +            tcg_out_movi(s, TCG_TYPE_TL, th, (is_sub ? -bh : bh));
> +            tcg_out_opc_reg(s, opc_add, th, ah, th);
> +        }

This value is currently constrained by 'M': +/- 0xfff.
You're suggesting that the fix should be to use 'I', which is signed 12-bit.

But this fix is definitely in the wrong place.


r~

Re: [RFC PATCH 1/3] tcg/riscv: Fix base regsiter for qemu_ld/st

[Philippe Mathieu-Daudé]

On 20/10/22 12:41, LIU Zhiwei wrote:
> When guest base is zero, we should use addr_regl as base regiser instead of

Typo "register" here and in patch subject.

> the initial register TCG_REG_TMP0.
> 
> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
> ---
>   tcg/riscv/tcg-target.c.inc | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/tcg/riscv/tcg-target.c.inc b/tcg/riscv/tcg-target.c.inc
> index 81a83e45b1..32f4bc7bfc 100644
> --- a/tcg/riscv/tcg-target.c.inc
> +++ b/tcg/riscv/tcg-target.c.inc
> @@ -1185,6 +1185,8 @@ static void tcg_out_qemu_ld(TCGContext *s, const TCGArg *args, bool is_64)
>       }
>       if (guest_base != 0) {
>           tcg_out_opc_reg(s, OPC_ADD, base, TCG_GUEST_BASE_REG, addr_regl);
> +    } else {
> +        base = addr_regl;
>       }
>       tcg_out_qemu_ld_direct(s, data_regl, data_regh, base, opc, is_64);
>   #endif
> @@ -1257,6 +1259,8 @@ static void tcg_out_qemu_st(TCGContext *s, const TCGArg *args, bool is_64)
>       }
>       if (guest_base != 0) {
>           tcg_out_opc_reg(s, OPC_ADD, base, TCG_GUEST_BASE_REG, addr_regl);
> +    } else {
> +        base = addr_regl;
>       }
>       tcg_out_qemu_st_direct(s, data_regl, data_regh, base, opc);
>   #endif

Maybe worth inverting the if statement, otherwise LGTM.

Re: [RFC PATCH 3/3] tcg/riscv: Remove a wrong optimization for addsub2

[Richard Henderson]

On 10/20/22 20:41, LIU Zhiwei wrote:
> It's not clear what it is doing here. And it's wrong because bl and
> al are both register, so we can't add them by an ADDI instruction.
> 
> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
> ---
>   tcg/riscv/tcg-target.c.inc | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/tcg/riscv/tcg-target.c.inc b/tcg/riscv/tcg-target.c.inc
> index bfdf2bea69..a07fd0864f 100644
> --- a/tcg/riscv/tcg-target.c.inc
> +++ b/tcg/riscv/tcg-target.c.inc
> @@ -705,9 +705,6 @@ static void tcg_out_addsub2(TCGContext *s,
>                   tcg_out_opc_reg(s, OPC_SLTU, TCG_REG_TMP0,
>                                   rl, al);
>               }
> -        } else if (rl == al && rl == bl) {
> -            tcg_out_opc_imm(s, OPC_SLTI, TCG_REG_TMP0, al, 0);
> -            tcg_out_opc_reg(s, opc_addi, rl, al, bl);
>           } else {

Removing this is incorrect; it's a simple typo for opc_add.

The case being required for rl == al == bl, which the following else will treat incorrectly.


r~

Re: [RFC PATCH 1/3] tcg/riscv: Fix base regsiter for qemu_ld/st

[LIU Zhiwei]

On 2022/10/20 19:18, Richard Henderson wrote:
> On 10/20/22 20:41, LIU Zhiwei wrote:
>> When guest base is zero, we should use addr_regl as base regiser 
>> instead of
>> the initial register TCG_REG_TMP0.
>>
>> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
>> ---
>>   tcg/riscv/tcg-target.c.inc | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/tcg/riscv/tcg-target.c.inc b/tcg/riscv/tcg-target.c.inc
>> index 81a83e45b1..32f4bc7bfc 100644
>> --- a/tcg/riscv/tcg-target.c.inc
>> +++ b/tcg/riscv/tcg-target.c.inc
>> @@ -1185,6 +1185,8 @@ static void tcg_out_qemu_ld(TCGContext *s, 
>> const TCGArg *args, bool is_64)
>>       }
>>       if (guest_base != 0) {
>>           tcg_out_opc_reg(s, OPC_ADD, base, TCG_GUEST_BASE_REG, 
>> addr_regl);
>> +    } else {
>> +        base = addr_regl;
>>       }
>
> You're right that there's a bug here, where TMP0 remains 
> uninitialized.  I think it would be better to reorg the other 
> direction: begin with initializeing base = addr_regl, 

Do you mean only in user mode? I see TCG_REG_TMP0 has been used in 
tcg_out_tlb_load when  system mode.

Best Regards,
Zhiwei

> and then change it away only when we make modifications.
>
> r~

Re: [RFC PATCH 1/3] tcg/riscv: Fix base regsiter for qemu_ld/st

[LIU Zhiwei]

On 2022/10/20 19:26, Philippe Mathieu-Daudé wrote:
> On 20/10/22 12:41, LIU Zhiwei wrote:
>> When guest base is zero, we should use addr_regl as base regiser 
>> instead of
>
> Typo "register" here and in patch subject.
>
>> the initial register TCG_REG_TMP0.
>>
>> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
>> ---
>>   tcg/riscv/tcg-target.c.inc | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/tcg/riscv/tcg-target.c.inc b/tcg/riscv/tcg-target.c.inc
>> index 81a83e45b1..32f4bc7bfc 100644
>> --- a/tcg/riscv/tcg-target.c.inc
>> +++ b/tcg/riscv/tcg-target.c.inc
>> @@ -1185,6 +1185,8 @@ static void tcg_out_qemu_ld(TCGContext *s, 
>> const TCGArg *args, bool is_64)
>>       }
>>       if (guest_base != 0) {
>>           tcg_out_opc_reg(s, OPC_ADD, base, TCG_GUEST_BASE_REG, 
>> addr_regl);
>> +    } else {
>> +        base = addr_regl;
>>       }
>>       tcg_out_qemu_ld_direct(s, data_regl, data_regh, base, opc, is_64);
>>   #endif
>> @@ -1257,6 +1259,8 @@ static void tcg_out_qemu_st(TCGContext *s, 
>> const TCGArg *args, bool is_64)
>>       }
>>       if (guest_base != 0) {
>>           tcg_out_opc_reg(s, OPC_ADD, base, TCG_GUEST_BASE_REG, 
>> addr_regl);
>> +    } else {
>> +        base = addr_regl;
>>       }
>>       tcg_out_qemu_st_direct(s, data_regl, data_regh, base, opc);
>>   #endif
>
> Maybe worth inverting the if statement, otherwise LGTM.

Thanks.

Is there any different? Sorry, I don't get why we should invert the if 
statement.

Best Regards,
Zhiwei

Re: [RFC PATCH 1/3] tcg/riscv: Fix base regsiter for qemu_ld/st

[Richard Henderson]

On 10/20/22 21:42, LIU Zhiwei wrote:
> 
> On 2022/10/20 19:18, Richard Henderson wrote:
>> On 10/20/22 20:41, LIU Zhiwei wrote:
>>> When guest base is zero, we should use addr_regl as base regiser instead of
>>> the initial register TCG_REG_TMP0.
>>>
>>> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
>>> ---
>>>   tcg/riscv/tcg-target.c.inc | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/tcg/riscv/tcg-target.c.inc b/tcg/riscv/tcg-target.c.inc
>>> index 81a83e45b1..32f4bc7bfc 100644
>>> --- a/tcg/riscv/tcg-target.c.inc
>>> +++ b/tcg/riscv/tcg-target.c.inc
>>> @@ -1185,6 +1185,8 @@ static void tcg_out_qemu_ld(TCGContext *s, const TCGArg *args, 
>>> bool is_64)
>>>       }
>>>       if (guest_base != 0) {
>>>           tcg_out_opc_reg(s, OPC_ADD, base, TCG_GUEST_BASE_REG, addr_regl);
>>> +    } else {
>>> +        base = addr_regl;
>>>       }
>>
>> You're right that there's a bug here, where TMP0 remains uninitialized.  I think it 
>> would be better to reorg the other direction: begin with initializeing base = addr_regl, 
> 
> Do you mean only in user mode? I see TCG_REG_TMP0 has been used in tcg_out_tlb_load when  
> system mode.

Well, yes, since that's what you're patching here...


r~

Re: [RFC PATCH 3/3] tcg/riscv: Remove a wrong optimization for addsub2

[LIU Zhiwei]

On 2022/10/20 19:31, Richard Henderson wrote:
> On 10/20/22 20:41, LIU Zhiwei wrote:
>> It's not clear what it is doing here. And it's wrong because bl and
>> al are both register, so we can't add them by an ADDI instruction.
>>
>> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
>> ---
>>   tcg/riscv/tcg-target.c.inc | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/tcg/riscv/tcg-target.c.inc b/tcg/riscv/tcg-target.c.inc
>> index bfdf2bea69..a07fd0864f 100644
>> --- a/tcg/riscv/tcg-target.c.inc
>> +++ b/tcg/riscv/tcg-target.c.inc
>> @@ -705,9 +705,6 @@ static void tcg_out_addsub2(TCGContext *s,
>>                   tcg_out_opc_reg(s, OPC_SLTU, TCG_REG_TMP0,
>>                                   rl, al);
>>               }
>> -        } else if (rl == al && rl == bl) {
>> -            tcg_out_opc_imm(s, OPC_SLTI, TCG_REG_TMP0, al, 0);
>> -            tcg_out_opc_reg(s, opc_addi, rl, al, bl);
>>           } else {
>
> Removing this is incorrect; it's a simple typo for opc_add.
>
> The case being required for rl == al == bl, which the following else 
> will treat incorrectly.

Thanks. Get it.

Zhiwei

>
>
> r~

Re: [RFC PATCH 2/3] tcg/riscv: Fix tcg_out_opc_imm when imm exceeds

[LIU Zhiwei]

On 2022/10/20 19:22, Richard Henderson wrote:
> On 10/20/22 20:41, LIU Zhiwei wrote:
>> TYPE-I immediate can only represent a signed 12-bit value. If immediate
>> exceed, mov it to an register.
>>
>> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
>> ---
>>   tcg/riscv/tcg-target.c.inc | 28 +++++++++++++++++++++++-----
>>   1 file changed, 23 insertions(+), 5 deletions(-)
>>
>> diff --git a/tcg/riscv/tcg-target.c.inc b/tcg/riscv/tcg-target.c.inc
>> index 32f4bc7bfc..bfdf2bea69 100644
>> --- a/tcg/riscv/tcg-target.c.inc
>> +++ b/tcg/riscv/tcg-target.c.inc
>> @@ -668,7 +668,12 @@ static void tcg_out_addsub2(TCGContext *s,
>>       if (!cbh) {
>>           tcg_out_opc_reg(s, (is_sub ? opc_sub : opc_add), th, ah, bh);
>>       } else if (bh != 0 || ah == rl) {
>> -        tcg_out_opc_imm(s, opc_addi, th, ah, (is_sub ? -bh : bh));
>> +        if (bh == sextract(bh, 0, 12)) {
>> +            tcg_out_opc_imm(s, opc_addi, th, ah, (is_sub ? -bh : bh));
>> +        } else {
>> +            tcg_out_movi(s, TCG_TYPE_TL, th, (is_sub ? -bh : bh));
>> +            tcg_out_opc_reg(s, opc_add, th, ah, th);
>> +        }
>
> This value is currently constrained by 'M': +/- 0xfff.
Thanks. I missed it.
> You're suggesting that the fix should be to use 'I', which is signed 
> 12-bit.
>
> But this fix is definitely in the wrong place.

OK. I will have a try to look for the correct place.

Best Regards,
Zhiwei

>
>
> r~

Re: [RFC PATCH 2/3] tcg/riscv: Fix tcg_out_opc_imm when imm exceeds

[LIU Zhiwei]

On 2022/10/20 19:22, Richard Henderson wrote:
> On 10/20/22 20:41, LIU Zhiwei wrote:
>> TYPE-I immediate can only represent a signed 12-bit value. If immediate
>> exceed, mov it to an register.
>>
>> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
>> ---
>>   tcg/riscv/tcg-target.c.inc | 28 +++++++++++++++++++++++-----
>>   1 file changed, 23 insertions(+), 5 deletions(-)
>>
>> diff --git a/tcg/riscv/tcg-target.c.inc b/tcg/riscv/tcg-target.c.inc
>> index 32f4bc7bfc..bfdf2bea69 100644
>> --- a/tcg/riscv/tcg-target.c.inc
>> +++ b/tcg/riscv/tcg-target.c.inc
>> @@ -668,7 +668,12 @@ static void tcg_out_addsub2(TCGContext *s,
>>       if (!cbh) {
>>           tcg_out_opc_reg(s, (is_sub ? opc_sub : opc_add), th, ah, bh);
>>       } else if (bh != 0 || ah == rl) {
>> -        tcg_out_opc_imm(s, opc_addi, th, ah, (is_sub ? -bh : bh));
>> +        if (bh == sextract(bh, 0, 12)) {
>> +            tcg_out_opc_imm(s, opc_addi, th, ah, (is_sub ? -bh : bh));
>> +        } else {
>> +            tcg_out_movi(s, TCG_TYPE_TL, th, (is_sub ? -bh : bh));
>> +            tcg_out_opc_reg(s, opc_add, th, ah, th);
>> +        }
>
> This value is currently constrained by 'M': +/- 0xfff.

I don't know why we need 'M'. Can I just use the constraint

C_O2_I4(r, r, rZ, rZ, rS, rS);

If we don't need ‘M’ constraint, I want to remove it in next version patch.

Thanks,
Zhiwei

> You're suggesting that the fix should be to use 'I', which is signed 
> 12-bit.
>
> But this fix is definitely in the wrong place.
>
>
> r~

Re: [RFC PATCH 2/3] tcg/riscv: Fix tcg_out_opc_imm when imm exceeds

[Richard Henderson]

[-- Attachment #1: Type: text/plain, Size: 1739 bytes --]

On Fri, 21 Oct 2022, 12:57 LIU Zhiwei, <zhiwei_liu@linux.alibaba.com> wrote:

>
> On 2022/10/20 19:22, Richard Henderson wrote:
> > On 10/20/22 20:41, LIU Zhiwei wrote:
> >> TYPE-I immediate can only represent a signed 12-bit value. If immediate
> >> exceed, mov it to an register.
> >>
> >> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
> >> ---
> >>   tcg/riscv/tcg-target.c.inc | 28 +++++++++++++++++++++++-----
> >>   1 file changed, 23 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/tcg/riscv/tcg-target.c.inc b/tcg/riscv/tcg-target.c.inc
> >> index 32f4bc7bfc..bfdf2bea69 100644
> >> --- a/tcg/riscv/tcg-target.c.inc
> >> +++ b/tcg/riscv/tcg-target.c.inc
> >> @@ -668,7 +668,12 @@ static void tcg_out_addsub2(TCGContext *s,
> >>       if (!cbh) {
> >>           tcg_out_opc_reg(s, (is_sub ? opc_sub : opc_add), th, ah, bh);
> >>       } else if (bh != 0 || ah == rl) {
> >> -        tcg_out_opc_imm(s, opc_addi, th, ah, (is_sub ? -bh : bh));
> >> +        if (bh == sextract(bh, 0, 12)) {
> >> +            tcg_out_opc_imm(s, opc_addi, th, ah, (is_sub ? -bh : bh));
> >> +        } else {
> >> +            tcg_out_movi(s, TCG_TYPE_TL, th, (is_sub ? -bh : bh));
> >> +            tcg_out_opc_reg(s, opc_add, th, ah, th);
> >> +        }
> >
> > This value is currently constrained by 'M': +/- 0xfff.
>
> I don't know why we need 'M'. Can I just use the constraint
>
> C_O2_I4(r, r, rZ, rZ, rS, rS);
>

I see the problem now. Look at the top of tcg_out_addsub2, where we
(conditionally) negate the constant.

We want to constrain the constant to be representable either positive or
negative, i.e not -4096..4095 but -4095..4095.  But we got the endpoints
wrong in tcg_target_const_match: 0xfff instead of 0x7ff.


r~

[-- Attachment #2: Type: text/html, Size: 2615 bytes --]