| Message ID | 20230808031143.50925-21-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | tcg: Introduce negsetcond opcodes | expand |
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... thanks -- PMM
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) ? -- PMM
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~
On 8/11/23 08:06, Richard Henderson wrote: > 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". This patch can be dropped entirely. TEST clears C (which cmp vs 0 would also do). I was mis-remembering INC/DEC which leave C unchanged and thus uninitialized wrt the current operation. r~
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); @@ -1437,7 +1437,7 @@ static void tcg_out_brcond(TCGContext *s, int rexw, TCGCond cond, TCGArg arg1, TCGArg arg2, int const_arg2, TCGLabel *label, bool small) { - tcg_out_cmp(s, arg1, arg2, const_arg2, rexw); + tcg_out_cmp(s, rexw, arg1, arg2, const_arg2, false); tcg_out_jxx(s, tcg_cond_to_jcc[cond], label, small); } @@ -1528,7 +1528,7 @@ static void tcg_out_setcond(TCGContext *s, int rexw, TCGCond cond, TCGArg dest, TCGArg arg1, TCGArg arg2, int const_arg2) { - tcg_out_cmp(s, arg1, arg2, const_arg2, rexw); + tcg_out_cmp(s, rexw, arg1, arg2, const_arg2, false); tcg_out_modrm(s, OPC_SETCC | tcg_cond_to_jcc[cond], 0, dest); tcg_out_ext8u(s, dest, dest); } @@ -1594,7 +1594,7 @@ static void tcg_out_movcond(TCGContext *s, int rexw, TCGCond cond, TCGReg dest, TCGReg c1, TCGArg c2, int const_c2, TCGReg v1) { - tcg_out_cmp(s, c1, c2, const_c2, rexw); + tcg_out_cmp(s, rexw, c1, c2, const_c2, false); tcg_out_cmov(s, cond, rexw, dest, v1); } @@ -1637,7 +1637,7 @@ static void tcg_out_clz(TCGContext *s, int rexw, TCGReg dest, TCGReg arg1, tgen_arithi(s, ARITH_XOR + rexw, dest, rexw ? 63 : 31, 0); /* Since we have destroyed the flags from BSR, we have to re-test. */ - tcg_out_cmp(s, arg1, 0, 1, rexw); + tcg_out_cmp(s, rexw, arg1, 0, 1, false); tcg_out_cmov(s, TCG_COND_EQ, rexw, dest, arg2); } }
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(-)