[DTrace-devel] [PATCH 3/3] Drop unused arg from dt_cg_zerosptr()

[Kris Van Hees]

On Tue, Aug 22, 2023 at 05:46:56PM -0400, eugene.loh--- via DTrace-devel wrote:
> From: Eugene Loh <eugene.loh at oracle.com>
> 
> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> ---
>  libdtrace/dt_cg.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index ea3745bc..a8f5077b 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -2530,7 +2530,7 @@ dt_cg_pop_stack(int reg, dt_irlist_t *dlp, dt_regset_t *drp)
>   * Store a pointer to the 'memory block of zeros' in reg.
>   */
>  static void
> -dt_cg_zerosptr(int reg, dt_irlist_t *dlp, dt_regset_t *drp)
> +dt_cg_zerosptr(int reg, dt_irlist_t *dlp)

I do not agree that this is a good idea.  Most functions that we have like this
that are called as part of the code generation pass dlp and drp because they
are the two things we need for code generation.  Granted, this particularly
function currently does not make use of the register set allocation functions,
but being a function that emits instructions, I think it makes sense to pass
in the default entities that are used for code generation.

Yes, there are function that do not stick to this, and they are a pain already.
I.e. the legacy codebase we work from has a mixture of functions that pass in
dlp and drp, and then some that pass in pcb, and some that use the global yypcb
variable.  That is a mess I would really like to clean up someday but it is a
rather big task and require =s some discussion first om what the proper way to
do this ought to be.

I don't think there is any benefit from dropping drp here, nor do I see any
drawback from having it.  My own experiences with making changes in the code
generator has shown a few times already that an implementation change can cause
a cascade of call site changes, and passing variables down a chain because at
some point in the call chain a value got dropped and we ended up needing it
later on.

I general I see the value of not passing variables that are not necessary, but
with the cg functions I prefer to keep dlp and drp.
function 

>  {
>  	dtrace_hdl_t	*dtp = yypcb->pcb_hdl;
>  	dt_ident_t	*zero_off = dt_dlib_get_var(dtp, "ZERO_OFF");
> @@ -2645,7 +2645,7 @@ dt_cg_prep_dvar_args(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp,
>  			emit(dlp,  BPF_MOV_REG(BPF_REG_4, dnp->dn_reg));
>  		else
>  			emit(dlp,  BPF_MOV_IMM(BPF_REG_4, 0));
> -		dt_cg_zerosptr(BPF_REG_5, dlp, drp);
> +		dt_cg_zerosptr(BPF_REG_5, dlp);
>  	} else if (idp->di_flags & DT_IDFLG_TLS) {
>  		/* thread-local variables */
>  		fn = "dt_get_tvar";
> @@ -2659,7 +2659,7 @@ dt_cg_prep_dvar_args(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp,
>  			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);
> +		dt_cg_zerosptr(BPF_REG_4, dlp);
>  	} else
>  		assert(0);
>  
> @@ -3160,7 +3160,7 @@ empty_args:
>  
>  	emit(dlp,  BPF_MOV_REG(BPF_REG_1, treg));
>  	emite(dlp, BPF_MOV_IMM(BPF_REG_2, -1), maxtupsz);
> -	dt_cg_zerosptr(BPF_REG_3, dlp, drp);
> +	dt_cg_zerosptr(BPF_REG_3, dlp);
>  	dt_regset_xalloc(drp, BPF_REG_0);
>  	emit(dlp,  BPF_CALL_HELPER(BPF_FUNC_probe_read));
>  	dt_regset_free_args(drp);
> @@ -7556,7 +7556,7 @@ dt_cg_agg(dt_pcb_t *pcb, dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>  		emit(dlp, BPF_MOV_IMM(BPF_REG_4, 0));
>  	}
>  
> -	dt_cg_zerosptr(BPF_REG_5, dlp, drp);
> +	dt_cg_zerosptr(BPF_REG_5, dlp);
>  
>  	dt_regset_xalloc(drp, BPF_REG_0);
>  	emite(dlp, BPF_CALL_FUNC(idp->di_id), idp);
> -- 
> 2.18.4
> 
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel