[DTrace-devel] [PATCH 08/12] cg: integrate dvar lookup into dt_cg_prep_dvar()

[Eugene Loh]

Reviewed-by: Eugene Loh <eugene.loh at oracle.com>

comments below...

On 1/5/24 00:29, Kris Van Hees via DTrace-devel wrote:
> The loading and storing of dynamic variables was implemented using a
> function dt_cg_prep_dvar_args() to prepare the arguments for a function
> call, followed by performing the actual function call in the respective
> callers.  Those calls are identical, so it makes sense to consolidate
> argument setup and call into a single function.
>
> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
>   libdtrace/dt_cg.c | 47 +++++++++++++++++++++++++++--------------------
>   1 file changed, 27 insertions(+), 20 deletions(-)
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 615ebdbf..39a3b4e8 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -2848,11 +2848,12 @@ dt_cg_arglist(dt_ident_t *idp, dt_node_t *args, dt_irlist_t *dlp,
>   	      dt_regset_t *drp);
>   
>   static void
> -dt_cg_prep_dvar_args(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp,
> -		     dt_ident_t *idp, int isstore, dt_ident_t **fnpp)
> +dt_cg_prep_dvar(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp,
> +		dt_ident_t *idp, int isstore)
>   {
>   	uint_t		varid = idp->di_id - DIF_VAR_OTHER_UBASE;
>   	const char	*fn;
> +	dt_ident_t	*fnp;
>   
>   	if (idp->di_kind == DT_IDENT_ARRAY) {
>   		/* Associative (global or TLS) array.  Cannot be in alloca space. */
> @@ -2869,9 +2870,14 @@ dt_cg_prep_dvar_args(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp,
>   		emit(dlp,  BPF_MOV_REG(BPF_REG_2, args->dn_args->dn_reg));
>   		dt_regset_free(drp, args->dn_args->dn_reg);
>   		emit(dlp,  BPF_MOV_IMM(BPF_REG_3, isstore));
> -		if (isstore)
> -			emit(dlp,  BPF_MOV_REG(BPF_REG_4, dnp->dn_reg));
> -		else
> +		if (isstore) {
> +			dt_node_t	*rp = dnp->dn_right;
> +
> +			if (rp->dn_kind == DT_NODE_INT && rp->dn_value == 0)
> +				emit(dlp,  BPF_MOV_IMM(BPF_REG_4, 0));
> +			else
> +				emit(dlp,  BPF_MOV_REG(BPF_REG_4, dnp->dn_reg));
> +		} else

What's the point of specializing the =0 case?  Explain in the commit 
message?  It's still a BPF instruction, so it isn't an optimization.  
Won't the old code just work here?  It's a bit more complexity and I do 
not see what it buys us.

Same for TLS...

>   			emit(dlp,  BPF_MOV_IMM(BPF_REG_4, 0));
>   		dt_cg_zerosptr(BPF_REG_5, dlp, drp);
>   	} else if (idp->di_flags & DT_IDFLG_TLS) {
> @@ -2883,16 +2889,26 @@ dt_cg_prep_dvar_args(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp,
>   
>   		emit(dlp,  BPF_MOV_IMM(BPF_REG_1, varid));
>   		emit(dlp,  BPF_MOV_IMM(BPF_REG_2, isstore));
> -		if (isstore)
> +		if (isstore) {
>   			emit(dlp,  BPF_MOV_REG(BPF_REG_3, dnp->dn_reg));

No need to assign %r3 since you're about to assign (overwrite) it 
anyhow.  (Or, just leave it if the funny "if =0" code is removed per my 
earlier comment.)

> -		else
> +			dt_node_t	*rp = dnp->dn_right;
> +
> +			if (rp->dn_kind == DT_NODE_INT && rp->dn_value == 0)
> +				emit(dlp,  BPF_MOV_IMM(BPF_REG_3, 0));
> +			else
> +				emit(dlp,  BPF_MOV_REG(BPF_REG_3, dnp->dn_reg));
> +		} else
>   			emit(dlp,  BPF_MOV_IMM(BPF_REG_3, 0));
>   		dt_cg_zerosptr(BPF_REG_4, dlp, drp);
>   	} else
>   		assert(0);
>   
> -	*fnpp = dt_dlib_get_func(yypcb->pcb_hdl, fn);
> -	assert(*fnpp);
> +	fnp = dt_dlib_get_func(yypcb->pcb_hdl, fn);
> +	assert(fnp);
> +
> +	dt_regset_xalloc(drp, BPF_REG_0);
> +	emite(dlp, BPF_CALL_FUNC(fnp->di_id), fnp);
> +	dt_regset_free_args(drp);
>   }
>   
>   static void
> @@ -2905,11 +2921,7 @@ dt_cg_load_var(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>   
>   	/* First handle dvars. */
>   	if (idp->di_kind == DT_IDENT_ARRAY || idp->di_flags & DT_IDFLG_TLS) {
> -		dt_cg_prep_dvar_args(dnp, dlp, drp, idp, 0, &fnp);
> -
> -		dt_regset_xalloc(drp, BPF_REG_0);
> -		emite(dlp, BPF_CALL_FUNC(fnp->di_id), fnp);
> -		dt_regset_free_args(drp);
> +		dt_cg_prep_dvar(dnp, dlp, drp, idp, 0);
>   
>   		if ((dnp->dn_reg = dt_regset_alloc(drp)) == -1)
>   			longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> @@ -3606,7 +3618,6 @@ dt_cg_store_var(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp,
>   {
>   	int	reg;
>   	size_t	size;
> -	dt_ident_t *fnp;
>   
>   	idp->di_flags |= DT_IDFLG_DIFW;
>   
> @@ -3631,11 +3642,7 @@ dt_cg_store_var(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp,
>   		uint_t		lbl_notnull = dt_irlist_label(dlp);
>   		dt_node_t	*rp = dnp->dn_right;
>   
> -		dt_cg_prep_dvar_args(dnp, dlp, drp, idp, 1, &fnp);
> -
> -		dt_regset_xalloc(drp, BPF_REG_0);
> -		emite(dlp, BPF_CALL_FUNC(fnp->di_id), fnp);
> -		dt_regset_free_args(drp);
> +		dt_cg_prep_dvar(dnp, dlp, drp, idp, 1);
>   
>   		/*
>   		 * Special case: a store of a literal 0 causes the dynamic

The comment block getting chopped off here in the diff is:
                 /*
                  * Special case: a store of a literal 0 causes the dynamic
                  * variable to be deleted by the BPF function called 
above, so
                  * nothing is left to be done.
                  */
The "BPF function called above" was already a little opaque (what is 
fnp->di_id?), and with this patch the call disappears entirely, at least 
from any obvious presence in the immediately preceding code.  Maybe 
replace "the BPF function called above" with "dt_cg_prep_dvar()".