1. Patchew
  2. QEMU
  3. [RFC PATCH 0/3] Fix some TCG RISC-V backend bugs
  4. View patch

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

LIU Zhiwei posted 3 patches 3 years, 3 months ago
Maintainers: Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <Alistair.Francis@wdc.com>, Richard Henderson <richard.henderson@linaro.org>
[RFC PATCH 2/3] tcg/riscv: Fix tcg_out_opc_imm when imm exceeds
Posted by LIU Zhiwei 3 years, 3 months ago 
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
Re: [RFC PATCH 2/3] tcg/riscv: Fix tcg_out_opc_imm when imm exceeds
Posted by Richard Henderson 3 years, 3 months ago 
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 2/3] tcg/riscv: Fix tcg_out_opc_imm when imm exceeds
Posted by LIU Zhiwei 3 years, 3 months ago 
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
Posted by Richard Henderson 3 years, 3 months ago 
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~
Re: [RFC PATCH 2/3] tcg/riscv: Fix tcg_out_opc_imm when imm exceeds
Posted by LIU Zhiwei 3 years, 3 months ago 
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~

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.