Re: [PATCH] tcg/aarch64: Fix constant subtraction in tcg_out_addsub2

[Richard Henderson]

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