Re: [PATCH v3 38/48] tcg/optimize: Split out fold_masks

[Richard Henderson]

On 10/26/21 8:32 AM, Alex Bennée wrote:
> Richard Henderson <richard.henderson@linaro.org> writes:
>
> > Move all of the known-zero optimizations into the per-opcode
> > functions.  Use fold_masks when there is a possibility of the
> > result being determined, and simply set ctx->z_mask otherwise.
> >
> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > ---
> >   tcg/optimize.c | 545 ++++++++++++++++++++++++++-----------------------
> >   1 file changed, 294 insertions(+), 251 deletions(-)
> >
> > diff --git a/tcg/optimize.c b/tcg/optimize.c
> > index 6c1cc3e635..f0086ee789 100644
> > --- a/tcg/optimize.c
> > +++ b/tcg/optimize.c
> > @@ -50,7 +50,8 @@ typedef struct OptContext {
> >       TCGTempSet temps_used;
> >   
> >       /* In flight values from optimization. */
> > -    uint64_t z_mask;
> > +    uint64_t a_mask;  /* mask bit is 0 iff value identical to first input */
> > +    uint64_t z_mask;  /* mask bit is 0 iff value bit is 0 */
>
> nit: too much iff?

Eh.  It's one iff per description.
I tried to find another way to write it, but struggled.
Plus iff lets it fit on one line.  ;-)

> > +    const TCGOpDef *def = &tcg_op_defs[op->opc];
> > +    MemOpIdx oi = op->args[def->nb_oargs + def->nb_iargs];
> > +    MemOp mop = get_memop(oi);
> > +    int width = 8 << (mop & MO_SIZE);
>
> Given we have a helper memop_size() it might be worth adding another
> memop_size_bits()?

Sure.
> > +static bool fold_tcg_ld(OptContext *ctx, TCGOp *op)
> > +{
> > +    /* We can't do any folding with a load, but we can record bits. */
> > +    switch (op->opc) {
> > +    CASE_OP_32_64(ld8u):
> > +        ctx->z_mask = 0xff;
> > +        break;
> > +    CASE_OP_32_64(ld16u):
> > +        ctx->z_mask = 0xffff;
> > +        break;
> > +    case INDEX_op_ld32u_i64:
> > +        ctx->z_mask = 0xffffffffu;
> > +        break;
> > +    default:
> > +        g_assert_not_reached();
> > +    }
> > +    return false;
>
> Given we use MAKE_64BIT_MASK elsewhere we should do here as well.

Sure.


r~