| Message ID | 20210228232847.322468-1-richard.henderson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tcg/aarch64: Fix constant subtraction in tcg_out_addsub2 | expand |
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
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 >
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
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(-)