* [RFC PATCH 0/3] Fix some TCG RISC-V backend bugs
@ 2022-10-20 10:41 LIU Zhiwei
2022-10-20 10:41 ` [RFC PATCH 1/3] tcg/riscv: Fix base regsiter for qemu_ld/st LIU Zhiwei
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: LIU Zhiwei @ 2022-10-20 10:41 UTC (permalink / raw)
To: qemu-devel, qemu-riscv
Cc: Alistair.Francis, palmer, bin.meng, richard.henderson, lzw194868,
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
^ permalink raw reply [flat|nested] 15+ messages in thread* [RFC PATCH 1/3] tcg/riscv: Fix base regsiter for qemu_ld/st 2022-10-20 10:41 [RFC PATCH 0/3] Fix some TCG RISC-V backend bugs LIU Zhiwei @ 2022-10-20 10:41 ` LIU Zhiwei 2022-10-20 11:18 ` Richard Henderson 2022-10-20 11:26 ` Philippe Mathieu-Daudé 2022-10-20 10:41 ` [RFC PATCH 2/3] tcg/riscv: Fix tcg_out_opc_imm when imm exceeds LIU Zhiwei 2022-10-20 10:41 ` [RFC PATCH 3/3] tcg/riscv: Remove a wrong optimization for addsub2 LIU Zhiwei 2 siblings, 2 replies; 15+ messages in thread From: LIU Zhiwei @ 2022-10-20 10:41 UTC (permalink / raw) To: qemu-devel, qemu-riscv Cc: Alistair.Francis, palmer, bin.meng, richard.henderson, lzw194868, 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 1/3] tcg/riscv: Fix base regsiter for qemu_ld/st 2022-10-20 10:41 ` [RFC PATCH 1/3] tcg/riscv: Fix base regsiter for qemu_ld/st LIU Zhiwei @ 2022-10-20 11:18 ` Richard Henderson 2022-10-20 11:42 ` LIU Zhiwei 2022-10-20 11:26 ` Philippe Mathieu-Daudé 1 sibling, 1 reply; 15+ messages in thread From: Richard Henderson @ 2022-10-20 11:18 UTC (permalink / raw) To: LIU Zhiwei, qemu-devel, qemu-riscv Cc: Alistair.Francis, palmer, bin.meng, lzw194868 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~ ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 1/3] tcg/riscv: Fix base regsiter for qemu_ld/st 2022-10-20 11:18 ` Richard Henderson @ 2022-10-20 11:42 ` LIU Zhiwei 2022-10-20 12:01 ` Richard Henderson 0 siblings, 1 reply; 15+ messages in thread From: LIU Zhiwei @ 2022-10-20 11:42 UTC (permalink / raw) To: Richard Henderson, qemu-devel, qemu-riscv Cc: Alistair.Francis, palmer, bin.meng, lzw194868 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~ ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 1/3] tcg/riscv: Fix base regsiter for qemu_ld/st 2022-10-20 11:42 ` LIU Zhiwei @ 2022-10-20 12:01 ` Richard Henderson 0 siblings, 0 replies; 15+ messages in thread From: Richard Henderson @ 2022-10-20 12:01 UTC (permalink / raw) To: LIU Zhiwei, qemu-devel, qemu-riscv Cc: Alistair.Francis, palmer, bin.meng, lzw194868 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~ ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 1/3] tcg/riscv: Fix base regsiter for qemu_ld/st 2022-10-20 10:41 ` [RFC PATCH 1/3] tcg/riscv: Fix base regsiter for qemu_ld/st LIU Zhiwei 2022-10-20 11:18 ` Richard Henderson @ 2022-10-20 11:26 ` Philippe Mathieu-Daudé 2022-10-20 11:44 ` LIU Zhiwei 1 sibling, 1 reply; 15+ messages in thread From: Philippe Mathieu-Daudé @ 2022-10-20 11:26 UTC (permalink / raw) To: LIU Zhiwei, qemu-devel, qemu-riscv Cc: Alistair.Francis, palmer, bin.meng, richard.henderson, lzw194868 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 1/3] tcg/riscv: Fix base regsiter for qemu_ld/st 2022-10-20 11:26 ` Philippe Mathieu-Daudé @ 2022-10-20 11:44 ` LIU Zhiwei 0 siblings, 0 replies; 15+ messages in thread From: LIU Zhiwei @ 2022-10-20 11:44 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel, qemu-riscv Cc: Alistair.Francis, palmer, bin.meng, richard.henderson, lzw194868 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [RFC PATCH 2/3] tcg/riscv: Fix tcg_out_opc_imm when imm exceeds 2022-10-20 10:41 [RFC PATCH 0/3] Fix some TCG RISC-V backend bugs LIU Zhiwei 2022-10-20 10:41 ` [RFC PATCH 1/3] tcg/riscv: Fix base regsiter for qemu_ld/st LIU Zhiwei @ 2022-10-20 10:41 ` LIU Zhiwei 2022-10-20 11:22 ` Richard Henderson 2022-10-20 10:41 ` [RFC PATCH 3/3] tcg/riscv: Remove a wrong optimization for addsub2 LIU Zhiwei 2 siblings, 1 reply; 15+ messages in thread From: LIU Zhiwei @ 2022-10-20 10:41 UTC (permalink / raw) To: qemu-devel, qemu-riscv Cc: Alistair.Francis, palmer, bin.meng, richard.henderson, lzw194868, 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 2/3] tcg/riscv: Fix tcg_out_opc_imm when imm exceeds 2022-10-20 10:41 ` [RFC PATCH 2/3] tcg/riscv: Fix tcg_out_opc_imm when imm exceeds LIU Zhiwei @ 2022-10-20 11:22 ` Richard Henderson 2022-10-20 12:42 ` LIU Zhiwei 2022-10-21 2:57 ` LIU Zhiwei 0 siblings, 2 replies; 15+ messages in thread From: Richard Henderson @ 2022-10-20 11:22 UTC (permalink / raw) To: LIU Zhiwei, qemu-devel, qemu-riscv Cc: Alistair.Francis, palmer, bin.meng, lzw194868 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~ ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 2/3] tcg/riscv: Fix tcg_out_opc_imm when imm exceeds 2022-10-20 11:22 ` Richard Henderson @ 2022-10-20 12:42 ` LIU Zhiwei 2022-10-21 2:57 ` LIU Zhiwei 1 sibling, 0 replies; 15+ messages in thread From: LIU Zhiwei @ 2022-10-20 12:42 UTC (permalink / raw) To: Richard Henderson, qemu-devel, qemu-riscv Cc: Alistair.Francis, palmer, bin.meng, lzw194868 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~ ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 2/3] tcg/riscv: Fix tcg_out_opc_imm when imm exceeds 2022-10-20 11:22 ` Richard Henderson 2022-10-20 12:42 ` LIU Zhiwei @ 2022-10-21 2:57 ` LIU Zhiwei 2022-10-21 4:29 ` Richard Henderson 1 sibling, 1 reply; 15+ messages in thread From: LIU Zhiwei @ 2022-10-21 2:57 UTC (permalink / raw) To: Richard Henderson, qemu-devel, qemu-riscv Cc: Alistair.Francis, palmer, bin.meng, lzw194868 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~ ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 2/3] tcg/riscv: Fix tcg_out_opc_imm when imm exceeds 2022-10-21 2:57 ` LIU Zhiwei @ 2022-10-21 4:29 ` Richard Henderson 0 siblings, 0 replies; 15+ messages in thread From: Richard Henderson @ 2022-10-21 4:29 UTC (permalink / raw) To: LIU Zhiwei Cc: qemu-devel@nongnu.org Developers, open list:RISC-V, Alistair Francis, Palmer Dabbelt, Bin Meng, lzw194868 [-- 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 --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* [RFC PATCH 3/3] tcg/riscv: Remove a wrong optimization for addsub2 2022-10-20 10:41 [RFC PATCH 0/3] Fix some TCG RISC-V backend bugs LIU Zhiwei 2022-10-20 10:41 ` [RFC PATCH 1/3] tcg/riscv: Fix base regsiter for qemu_ld/st LIU Zhiwei 2022-10-20 10:41 ` [RFC PATCH 2/3] tcg/riscv: Fix tcg_out_opc_imm when imm exceeds LIU Zhiwei @ 2022-10-20 10:41 ` LIU Zhiwei 2022-10-20 11:31 ` Richard Henderson 2 siblings, 1 reply; 15+ messages in thread From: LIU Zhiwei @ 2022-10-20 10:41 UTC (permalink / raw) To: qemu-devel, qemu-riscv Cc: Alistair.Francis, palmer, bin.meng, richard.henderson, lzw194868, 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 3/3] tcg/riscv: Remove a wrong optimization for addsub2 2022-10-20 10:41 ` [RFC PATCH 3/3] tcg/riscv: Remove a wrong optimization for addsub2 LIU Zhiwei @ 2022-10-20 11:31 ` Richard Henderson 2022-10-20 12:39 ` LIU Zhiwei 0 siblings, 1 reply; 15+ messages in thread From: Richard Henderson @ 2022-10-20 11:31 UTC (permalink / raw) To: LIU Zhiwei, qemu-devel, qemu-riscv Cc: Alistair.Francis, palmer, bin.meng, lzw194868 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~ ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 3/3] tcg/riscv: Remove a wrong optimization for addsub2 2022-10-20 11:31 ` Richard Henderson @ 2022-10-20 12:39 ` LIU Zhiwei 0 siblings, 0 replies; 15+ messages in thread From: LIU Zhiwei @ 2022-10-20 12:39 UTC (permalink / raw) To: Richard Henderson, qemu-devel, qemu-riscv Cc: Alistair.Francis, palmer, bin.meng, lzw194868 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~ ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2022-10-21 4:30 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-10-20 10:41 [RFC PATCH 0/3] Fix some TCG RISC-V backend bugs LIU Zhiwei 2022-10-20 10:41 ` [RFC PATCH 1/3] tcg/riscv: Fix base regsiter for qemu_ld/st LIU Zhiwei 2022-10-20 11:18 ` Richard Henderson 2022-10-20 11:42 ` LIU Zhiwei 2022-10-20 12:01 ` Richard Henderson 2022-10-20 11:26 ` Philippe Mathieu-Daudé 2022-10-20 11:44 ` LIU Zhiwei 2022-10-20 10:41 ` [RFC PATCH 2/3] tcg/riscv: Fix tcg_out_opc_imm when imm exceeds LIU Zhiwei 2022-10-20 11:22 ` Richard Henderson 2022-10-20 12:42 ` LIU Zhiwei 2022-10-21 2:57 ` LIU Zhiwei 2022-10-21 4:29 ` Richard Henderson 2022-10-20 10:41 ` [RFC PATCH 3/3] tcg/riscv: Remove a wrong optimization for addsub2 LIU Zhiwei 2022-10-20 11:31 ` Richard Henderson 2022-10-20 12:39 ` LIU Zhiwei
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.