| Message ID | 20221020104154.4276-3-zhiwei_liu@linux.alibaba.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix some TCG RISC-V backend bugs | expand |
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~
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~
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~
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~
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);
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(-)