Re: [PATCH 20/24] tcg/i386: Add cf parameter to tcg_out_cmp

[Richard Henderson]

On 8/11/23 03:45, Peter Maydell wrote:
> On Fri, 11 Aug 2023 at 11:26, Peter Maydell <peter.maydell@linaro.org> wrote:
> > On Tue, 8 Aug 2023 at 04:13, Richard Henderson
> > <richard.henderson@linaro.org> wrote:
> > > Add the parameter to avoid TEST and pass along to tgen_arithi.
> > > All current users pass false.
> > >
> > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > > ---
> > >   tcg/i386/tcg-target.c.inc | 16 ++++++++--------
> > >   1 file changed, 8 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/tcg/i386/tcg-target.c.inc b/tcg/i386/tcg-target.c.inc
> > > index b88fc14afd..56549ff2a0 100644
> > > --- a/tcg/i386/tcg-target.c.inc
> > > +++ b/tcg/i386/tcg-target.c.inc
> > > @@ -1418,15 +1418,15 @@ static void tcg_out_jxx(TCGContext *s, int opc, 
> > > TCGLabel *l, bool small)
> > >       }
> > >   }
> > >
> > > -static void tcg_out_cmp(TCGContext *s, TCGArg arg1, TCGArg arg2,
> > > -                        int const_arg2, int rexw)
> > > +static void tcg_out_cmp(TCGContext *s, int rexw, TCGArg arg1, TCGArg arg2,
> > > +                        int const_arg2, bool cf)
> > >   {
> > >       if (const_arg2) {
> > > -        if (arg2 == 0) {
> > > +        if (arg2 == 0 && !cf) {
> > >               /* test r, r */
> > >               tcg_out_modrm(s, OPC_TESTL + rexw, arg1, arg1);
> > >           } else {
> > > -            tgen_arithi(s, ARITH_CMP + rexw, arg1, arg2, 0);
> > > +            tgen_arithi(s, ARITH_CMP + rexw, arg1, arg2, cf);
> > >           }
> > >       } else {
> > >           tgen_arithr(s, ARITH_CMP + rexw, arg1, arg2);
> >
> > I don't really understand the motivation here.
> > Why are some uses of this function fine with using the TEST
> > insn, but some must avoid it? What does 'cf' stand for?
> > A comment would help here if there isn't a clearer argument
> > name available...
>
> Looking at the following patch suggests perhaps:
>
> /**
>   * tcg_out_cmp: Emit a compare, setting the X, Y, Z flags accordingly.
>   * @need_cf : true if the comparison must also set CF
>   */
>
> (fill in which XYZ flags you can rely on even if need_cf is false)

I can add that, yes.

Basically, test sets SZ flags, where cmp sets SZCO.  I want to add an optimizaton using C, 
so "cmp 0,x" should not be silently replaced by "test x,x".


r~