[DTrace-devel] [PATCH 1/4] clean up dt_cg_typecast(): unnecessary move from register to itself

[Kris Van Hees]

On Tue, Aug 11, 2020 at 06:11:09PM -0400, eugene.loh at oracle.com wrote:
> From: Eugene Loh <eugene.loh at oracle.com>
> 
> The unnecessary move is here:
>     1147    instr = BPF_MOV_REG(dst->dn_reg, src->dn_reg);
>     1148    dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> 
> This move is unnecessary because dt_cg_typeset()'s callers always
> set dst->dn_reg=src->dn_reg:
> 
>     $ grep -B 1 dt_cg_typecast dtrace-user/libdtrace/dt_cg.c
>     static void
>     dt_cg_typecast(const dt_node_t *src, const dt_node_t *dst,
>     --
>         isp->dis_args[i].dn_reg = dnp->dn_reg; /* re-use register */
>         dt_cg_typecast(dnp, &isp->dis_args[i], dlp, drp);
>     --
>         mnp->dn_reg = mnp->dn_membexpr->dn_reg;
>         dt_cg_typecast(mnp->dn_membexpr, mnp, dlp, drp);
>     --
>         dnp->dn_reg = inp->din_root->dn_reg;
>         dt_cg_typecast(inp->din_root, dnp, dlp, drp);
>     --
>         dnp->dn_reg = dnp->dn_right->dn_reg;
>         dt_cg_typecast(dnp->dn_right, dnp, dlp, drp);
>     --
>         dnp->dn_reg = mnp->dn_membexpr->dn_reg;
>         dt_cg_typecast(mnp->dn_membexpr, dnp, dlp, drp);
> 
> Indeed, dt_cg_typecast() expects src and dst will already be in
> the same register.  For example, there is no move from src->dn_reg
> to dst->dn_reg if no extra typecast is needed.  And historically,
> that register move never existed;  it was introduced in the port
> to BPF:
> 
>     % git show ad8e9312f5c2
>     [...]
>     Convert the D compiler to generate BPF
>     [...]
>     diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
>     [...]
>     @@ -464,12 +482,13 @@ dt_cg_typecast(const dt_node_t *src, const dt_node_t *dst,
>     [...]
> 
>     +   instr = BPF_MOV_REG(dst->dn_reg, src->dn_reg);
>     +   dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
>     -   instr = DIF_INSTR_FMT(DIF_OP_SLL,
>     -       src->dn_reg, reg, dst->dn_reg);
>     +   instr = BPF_ALU64_REG(BPF_LSH, dst->dn_reg, reg);
>         dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> 
>     -   instr = DIF_INSTR_FMT((dst->dn_flags & DT_NF_SIGNED) ?
>     -       DIF_OP_SRA : DIF_OP_SRL, dst->dn_reg, reg, dst->dn_reg);
>     +   instr = BPF_ALU64_REG((dst->dn_flags & DT_NF_SIGNED) ?
>     +       BPF_ARSH : BPF_RSH, dst->dn_reg, reg);
> 
>         dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
>         dt_regset_free(drp, reg);
> 
> Note that the left- and right-shift instructions are converted to
> BPF;  meanwhile, the BPF_MOV_REG() instruction is introduced with
> no precedent in legacy DTrace.

That is not true...  as you can see in the code you quote, the DIF operation
to perform the shift uses a three-operand instruction using three registers:
one for the source value, one for the shift, and one for the destination.

BPF provides a two-operand shift instruction that modifies the value in the
provided register, as in: dst <<= reg.  So, to implement SLL (3-operand shigy
in DIF) in BPF, you first need to move src to dst, i.e.

	DIF: dst = src << reg;
	BPF: dst = src; dst <<= reg;

As such, in the general case for implementing SLL, you need a MOV + SHL in BPF.

The move was there in legacy DTrace, but it was part of the SLL implementation.
DIF-to-BPF cannot be done 1-to-1 at the instruction level because the
instruction sets are quite different.

As far as whether we should do this optimization here...  Let me think about
that a bit.  I plan on doing an optimization step later on before we do the
final assembly, and that would get rid of any MOV %rX, %rX already that tend to
be intrroduced in other places as well.  And even though the current uses do
not show a case where src != dst, I am not too sure that will continue to be
the case as we get further into implementing various language features and/or
code generation improvements.

E.g. it is possible we can do better code generation by using the value of an
expression that gets stored into a TLS variable directly from the register
that was used for the store instruvction rather than doing a fetch from the
TLS variable to get the value.  In that case, if the store to the TLS variable
involved a typecast of a value we still use afterwards. we definitely wouldn't
want the typecast to change the source register.

> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> ---
>  libdtrace/dt_cg.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 0b8cd1d6..4f657967 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -1144,8 +1144,6 @@ dt_cg_typecast(const dt_node_t *src, const dt_node_t *dst,
>  
>  		dt_cg_setx(dlp, reg, n);
>  
> -		instr = BPF_MOV_REG(dst->dn_reg, src->dn_reg);
> -		dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
>  		instr = BPF_ALU64_REG(BPF_LSH, dst->dn_reg, reg);
>  		dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
>  
> -- 
> 2.18.4
> 
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel