* [PATCH] tcg/aarch64: Fix constant subtraction in tcg_out_addsub2
@ 2021-02-28 23:28 Richard Henderson
2021-03-05 11:45 ` Peter Maydell
0 siblings, 1 reply; 3+ messages in thread
From: Richard Henderson @ 2021-02-28 23:28 UTC (permalink / raw)
To: qemu-devel
An hppa guest executing
0x000000000000e05c: ldil L%10000,r4
0x000000000000e060: ldo 0(r4),r4
0x000000000000e064: sub r3,r4,sp
produces
---- 000000000000e064 000000000000e068
sub2_i32 tmp0,tmp4,r3,$0x1,$0x10000,$0x0
after folding and constant propagation. Then we hit
tcg-target.c.inc:640: tcg_out_insn_3401: Assertion `aimm <= 0xfff' failed.
because aimm is in fact -16, but unsigned.
The ((bl < 0) ^ sub) condition which negates bl is incorrect and will
always lead to this abort. If the constant is positive, sub will make
it negative; if the constant is negative, sub will keep it negative.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
tcg/aarch64/tcg-target.c.inc | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/tcg/aarch64/tcg-target.c.inc b/tcg/aarch64/tcg-target.c.inc
index 1376cdc404..ec0a86d9d8 100644
--- a/tcg/aarch64/tcg-target.c.inc
+++ b/tcg/aarch64/tcg-target.c.inc
@@ -1410,10 +1410,10 @@ static void tcg_out_addsubi(TCGContext *s, int ext, TCGReg rd,
}
}
-static inline void tcg_out_addsub2(TCGContext *s, TCGType ext, TCGReg rl,
- TCGReg rh, TCGReg al, TCGReg ah,
- tcg_target_long bl, tcg_target_long bh,
- bool const_bl, bool const_bh, bool sub)
+static void tcg_out_addsub2(TCGContext *s, TCGType ext, TCGReg rl,
+ TCGReg rh, TCGReg al, TCGReg ah,
+ tcg_target_long bl, tcg_target_long bh,
+ bool const_bl, bool const_bh, bool sub)
{
TCGReg orig_rl = rl;
AArch64Insn insn;
@@ -1423,11 +1423,13 @@ static inline void tcg_out_addsub2(TCGContext *s, TCGType ext, TCGReg rl,
}
if (const_bl) {
- insn = I3401_ADDSI;
- if ((bl < 0) ^ sub) {
- insn = I3401_SUBSI;
+ if (bl < 0) {
bl = -bl;
+ insn = sub ? I3401_ADDSI : I3401_SUBSI;
+ } else {
+ insn = sub ? I3401_SUBSI : I3401_ADDSI;
}
+
if (unlikely(al == TCG_REG_XZR)) {
/* ??? We want to allow al to be zero for the benefit of
negation via subtraction. However, that leaves open the
--
2.25.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] tcg/aarch64: Fix constant subtraction in tcg_out_addsub2
2021-02-28 23:28 [PATCH] tcg/aarch64: Fix constant subtraction in tcg_out_addsub2 Richard Henderson
@ 2021-03-05 11:45 ` Peter Maydell
2021-03-05 15:13 ` Richard Henderson
0 siblings, 1 reply; 3+ messages in thread
From: Peter Maydell @ 2021-03-05 11:45 UTC (permalink / raw)
To: Richard Henderson; +Cc: QEMU Developers
On Sun, 28 Feb 2021 at 23:55, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> An hppa guest executing
>
> 0x000000000000e05c: ldil L%10000,r4
> 0x000000000000e060: ldo 0(r4),r4
> 0x000000000000e064: sub r3,r4,sp
>
> produces
>
> ---- 000000000000e064 000000000000e068
> sub2_i32 tmp0,tmp4,r3,$0x1,$0x10000,$0x0
>
> after folding and constant propagation. Then we hit
>
> tcg-target.c.inc:640: tcg_out_insn_3401: Assertion `aimm <= 0xfff' failed.
>
> because aimm is in fact -16, but unsigned.
>
> The ((bl < 0) ^ sub) condition which negates bl is incorrect and will
> always lead to this abort. If the constant is positive, sub will make
> it negative; if the constant is negative, sub will keep it negative.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> -static inline void tcg_out_addsub2(TCGContext *s, TCGType ext, TCGReg rl,
> - TCGReg rh, TCGReg al, TCGReg ah,
> - tcg_target_long bl, tcg_target_long bh,
> - bool const_bl, bool const_bh, bool sub)
> +static void tcg_out_addsub2(TCGContext *s, TCGType ext, TCGReg rl,
> + TCGReg rh, TCGReg al, TCGReg ah,
> + tcg_target_long bl, tcg_target_long bh,
> + bool const_bl, bool const_bh, bool sub)
> {
> TCGReg orig_rl = rl;
> AArch64Insn insn;
Seems like an unrelated change ?
> @@ -1423,11 +1423,13 @@ static inline void tcg_out_addsub2(TCGContext *s, TCGType ext, TCGReg rl,
> }
>
> if (const_bl) {
> - insn = I3401_ADDSI;
> - if ((bl < 0) ^ sub) {
> - insn = I3401_SUBSI;
> + if (bl < 0) {
> bl = -bl;
> + insn = sub ? I3401_ADDSI : I3401_SUBSI;
> + } else {
> + insn = sub ? I3401_SUBSI : I3401_ADDSI;
> }
> +
> if (unlikely(al == TCG_REG_XZR)) {
> /* ??? We want to allow al to be zero for the benefit of
> negation via subtraction. However, that leaves open the
> --
> 2.25.1
Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] tcg/aarch64: Fix constant subtraction in tcg_out_addsub2
2021-03-05 11:45 ` Peter Maydell
@ 2021-03-05 15:13 ` Richard Henderson
0 siblings, 0 replies; 3+ messages in thread
From: Richard Henderson @ 2021-03-05 15:13 UTC (permalink / raw)
To: Peter Maydell; +Cc: QEMU Developers
On 3/5/21 3:45 AM, Peter Maydell wrote:
> On Sun, 28 Feb 2021 at 23:55, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> An hppa guest executing
>>
>> 0x000000000000e05c: ldil L%10000,r4
>> 0x000000000000e060: ldo 0(r4),r4
>> 0x000000000000e064: sub r3,r4,sp
>>
>> produces
>>
>> ---- 000000000000e064 000000000000e068
>> sub2_i32 tmp0,tmp4,r3,$0x1,$0x10000,$0x0
>>
>> after folding and constant propagation. Then we hit
>>
>> tcg-target.c.inc:640: tcg_out_insn_3401: Assertion `aimm <= 0xfff' failed.
>>
>> because aimm is in fact -16, but unsigned.
>>
>> The ((bl < 0) ^ sub) condition which negates bl is incorrect and will
>> always lead to this abort. If the constant is positive, sub will make
>> it negative; if the constant is negative, sub will keep it negative.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>> -static inline void tcg_out_addsub2(TCGContext *s, TCGType ext, TCGReg rl,
>> - TCGReg rh, TCGReg al, TCGReg ah,
>> - tcg_target_long bl, tcg_target_long bh,
>> - bool const_bl, bool const_bh, bool sub)
>> +static void tcg_out_addsub2(TCGContext *s, TCGType ext, TCGReg rl,
>> + TCGReg rh, TCGReg al, TCGReg ah,
>> + tcg_target_long bl, tcg_target_long bh,
>> + bool const_bl, bool const_bh, bool sub)
>> {
>> TCGReg orig_rl = rl;
>> AArch64Insn insn;
>
> Seems like an unrelated change ?
Mm. Perhaps I should remove all of the inline markers all at once.
r~
>
>> @@ -1423,11 +1423,13 @@ static inline void tcg_out_addsub2(TCGContext *s, TCGType ext, TCGReg rl,
>> }
>>
>> if (const_bl) {
>> - insn = I3401_ADDSI;
>> - if ((bl < 0) ^ sub) {
>> - insn = I3401_SUBSI;
>> + if (bl < 0) {
>> bl = -bl;
>> + insn = sub ? I3401_ADDSI : I3401_SUBSI;
>> + } else {
>> + insn = sub ? I3401_SUBSI : I3401_ADDSI;
>> }
>> +
>> if (unlikely(al == TCG_REG_XZR)) {
>> /* ??? We want to allow al to be zero for the benefit of
>> negation via subtraction. However, that leaves open the
>> --
>> 2.25.1
>
> Otherwise
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-03-05 15:34 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-02-28 23:28 [PATCH] tcg/aarch64: Fix constant subtraction in tcg_out_addsub2 Richard Henderson
2021-03-05 11:45 ` Peter Maydell
2021-03-05 15:13 ` Richard Henderson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).