[DTrace-devel] [PATCH 08/12] cg: integrate dvar lookup into dt_cg_prep_dvar()
Kris Van Hees
kris.van.hees at oracle.com
Tue Jan 9 00:37:38 UTC 2024
On Mon, Jan 08, 2024 at 05:02:28PM -0500, Eugene Loh via DTrace-devel wrote:
> 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...
The main point is that dnp->dn_reg at this point may not have the correct value
(it might be -1 for some uses). Well, not quite at this point in the patch
series but it will. And since I knew it would get there and also that there is
value in pointing out the special case of assigning 0, I made the distinction
here already.
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.)
Mistake - should have been removed because the below code replaces it.
> > - 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()".
>
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel
More information about the DTrace-devel
mailing list