Re: [RFC bpf-next 2/2] bpf, mips: Add support for BPF_MOVSX in the JITs

[Johan Almbladh]

Hi Nicholas,

The MOV/MOVSX structure looks good in general. Some remarks on the
implementation of individual emittor functions.

> +/* Register move operation (32-bit), optionally with sign extension */
>  static void emit_mov_r32(struct jit_context *ctx, const u8 dst[],
> -                        const u8 src[])
> +                        const u8 src[], s16 off)
>  {
> -       emit_mov_r(ctx, lo(dst), lo(src));
> +       switch (off) {
> +       case 8:
> +               emit(ctx, sll, lo(dst), lo(src), 24);
> +               emit(ctx, sra, lo(dst), lo(dst), 24);
> +               clobber_reg(ctx, lo(dst));
> +               break;
> +       case 16:
> +               emit(ctx, sll, lo(dst), lo(src), 16);
> +               emit(ctx, sra, lo(dst), lo(dst), 16);
> +               clobber_reg(ctx, lo(dst));
> +               break;
> +       default:
> +               emit_mov_r(ctx, lo(dst), lo(src));
> +               break;
> +       }
>         emit_zext_ver(ctx, dst);
>  }

I would move the clobber marker to the end to reduce code duplication.
That is the pattern used in other places in the jit. Also consider
collapsing the cases off=8 and off=16 into one, with the shift
argument computed as 32 - off. That would parameterize the shift logic
instead of duplicating it.

The above applies to all four variants of the mov emittor.

> +/* Register move operation (64-bit), optionally with sign extension */
>  static void emit_mov_r64(struct jit_context *ctx, const u8 dst[],
> -                        const u8 src[])
> +                        const u8 src[], s16 off)
>  {
> -       emit_mov_r(ctx, lo(dst), lo(src));
> -       emit_mov_r(ctx, hi(dst), hi(src));
> +       switch (off) {
> +       case 8:
> +               emit(ctx, sll, lo(dst), lo(src), 24);
> +               emit(ctx, sra, lo(dst), lo(dst), 24);
> +               break;
> +       case 16:
> +               emit(ctx, sll, lo(dst), lo(src), 16);
> +               emit(ctx, sra, lo(dst), lo(dst), 16);
> +               break;
> +       case 32:
> +               emit(ctx, move, lo(dst), lo(src));
> +               break;
> +       default:
> +               emit_mov_r(ctx, lo(dst), lo(src));
> +               emit_mov_r(ctx, hi(dst), hi(src));
> +               return;
> +       }
> +       clobber_reg(ctx, lo(dst));
> +       emit(ctx, sra, hi(dst), lo(dst), 31);
> +       clobber_reg(ctx, hi(dst));
>  }

I have tried to avoid returns in the middle like this. If you combine
cases 8 and 16 as suggested, you can put the sra sign extension into
cases 8/16 and 32, and get rid of the return in the default case.

> -static void emit_mov_r64(struct jit_context *ctx, u8 dst, u8 src)
> +/* Register move operation (64-bit), optionally with sign extension */
> +static void emit_mov_r64(struct jit_context *ctx, u8 dst, u8 src, s16 off)
>  {
> -       emit_mov_r(ctx, dst, src);
> +       switch (off) {
> +       case 8:
> +               emit(ctx, dsll32, dst, src, 24);
> +               emit(ctx, dsra32, dst, dst, 24);
> +               break;
> +       case 16:
> +               emit(ctx, dsll32, dst, src, 16);
> +               emit(ctx, dsra32, dst, dst, 16);
> +               break;
> +       case 32:
> +               emit(ctx, sll, dst, src, 0);
> +               break;
> +       default:
> +               emit_mov_r(ctx, dst, src);
> +               return;

This return is not needed. Replace with break. Consider using the
emit_sext helper instead of raw sll to b better convey intent.

Thanks,
Johan