[DTrace-devel] [PATCH 09/10] Improve and unify dt_cg_prearith() and dt_cg_postarith()
Kris Van Hees
kris.van.hees at oracle.com
Tue Mar 22 22:43:10 UTC 2022
On Tue, Mar 22, 2022 at 03:34:29PM -0400, Eugene Loh via DTrace-devel wrote:
> Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
Thanks.
> On 3/18/22 3:04 PM, Kris Van Hees via DTrace-devel wrote:
> > Combine dt_cg_prearith() and dt_cg_postarith() into a single function
> > given how much functionality they share. The implementation is now
> > evaluating the child node one as an lvalue, to be used to load the
> > original value and also to store the new value. Associative arrays
> > and TLS variables are a special case because they will yield a NULL
> > lvalue if the variable was never assigned to (or if 0 was assigned to
> > it). In that case, we use dt_cg_store_var() to store the new value.
> is now evaluating -> now evaluates
> child node one -> child node
Thanks.
> > Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> > ---
> > libdtrace/dt_cg.c | 170 +++++++++++++++++++++-------------------------
> > 1 file changed, 76 insertions(+), 94 deletions(-)
> >
> > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > index 2b117889..3750cfa8 100644
> > --- a/libdtrace/dt_cg.c
> > +++ b/libdtrace/dt_cg.c
> > @@ -2825,124 +2825,106 @@ dt_cg_arithmetic_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp,
> > dt_cg_ptrsize(dnp->dn_right, dlp, drp, BPF_DIV, dnp->dn_reg);
> > }
> > -#ifdef FIXME
> > -static uint_t
> > -dt_cg_stvar(const dt_ident_t *idp)
> > +static void
> > +dt_cg_incdec_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp, uint_t op,
> > + int post)
> > {
> > - static const uint_t aops[] = { DIF_OP_STGAA, DIF_OP_STTAA, DIF_OP_NOP };
> > - static const uint_t sops[] = { DIF_OP_STGS, DIF_OP_STTS, DIF_OP_STLS };
> > + size_t size = dt_node_type_size(dnp->dn_child);
> > + ssize_t adj = 1;
> > + int is_dvar = 0;
> > + uint_t rbit = dnp->dn_child->dn_flags & DT_NF_REF;
> > + uint_t lbl_dflt, lbl_done;
> > + dt_ident_t *idp = NULL;
> > - uint_t i = (((idp->di_flags & DT_IDFLG_LOCAL) != 0) << 1) |
> > - ((idp->di_flags & DT_IDFLG_TLS) != 0);
> > + TRACE_REGSET(" incdec: Begin");
> > - return idp->di_kind == DT_IDENT_ARRAY ? aops[i] : sops[i];
> > -}
> > -#endif
> > -
> > -static void
> > -dt_cg_prearith_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp, uint_t op)
> > -{
> > - ctf_file_t *ctfp = dnp->dn_ctfp;
> > - ctf_id_t type;
> > - ssize_t size = 1;
> > + assert(size > 0 && size <= 8 &&
> > + (size & (size - 1)) == 0);
>
> Why assert() over two lines? Or again just introduce a macro for this
> common assertion and use it instead.
Mainly because we have been doing that in other instances of these asserts.
And yes, we should introduce a macro and replace them all, but not in this
patch because that is more generic.
> > if (dt_node_is_pointer(dnp)) {
> > + ctf_file_t *ctfp = dnp->dn_ctfp;
> > + ctf_id_t type;
> > +
> > type = ctf_type_resolve(ctfp, dnp->dn_type);
> > assert(ctf_type_kind(ctfp, type) == CTF_K_POINTER);
> > - size = ctf_type_size(ctfp, ctf_type_reference(ctfp, type));
> > + adj = ctf_type_size(ctfp, ctf_type_reference(ctfp, type));
> > }
> > - dt_cg_node(dnp->dn_child, dlp, drp);
> > - dnp->dn_reg = dnp->dn_child->dn_reg;
> > + assert(dnp->dn_child->dn_flags & DT_NF_WRITABLE);
> > + assert(dnp->dn_child->dn_flags & DT_NF_LVALUE);
> > - emit(dlp, BPF_ALU64_IMM(op, dnp->dn_reg, size));
> > + dnp->dn_reg = dt_regset_alloc(drp);
> > + if (dnp->dn_reg == -1)
> > + longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> > +
> > + dnp->dn_child->dn_flags |= DT_NF_REF; /* force pass-by-ref */
> > + dt_cg_node(dnp->dn_child, dlp, drp);
> Add
> dnp->dn_child->dn_flags &= ~DT_NF_REF;
> dnp->dn_child->dn_flags |= rbit;
>
> Best to restore as soon as possible rather than to leave things hanging.
But that isn't really appropriate from a semantics perspective because for the
remainder of the function, dnp->dn_child *is* a pass-by-ref entity.
> > /*
> > - * If we are modifying a variable, generate a store instruction for
> > - * the variable specified by the identifier. If we are storing to a
> > - * memory address, generate code again for the left-hand side using
> > - * DT_NF_REF to get the address, and then generate a store to it.
> > - * In both paths, we store the value in dnp->dn_reg (the new value).
> > + * If the lvalue is a dynamic variable (TLS or associative array
> > + * element), it may not have been created yet. The lvalue pointer will
> > + * be NULL in that case, so we need special handling for it.
> > + *
> > + * If not, we need to perform a NULL check.
>
> This comment block does not seem very clear, I think because it does not
> clearly match the actual code.
>
> E.g. what does "If not" mean? From the comment block, it apparently means
> "if the lvalue is not a dvar" but the code means "if not DT_NODE_VAR".
> Also, the "special handling" is coming later; I realize you mention it now
> to justify setting is_dvar, but it's more detail than is needed at this
> point.
>
> For the comment block, I recommend simply:
> /*
> * If the lvalue is a variable, determine if it is a dynamic
> variable
> * (TLS or associative array element). If not, perform a NULL
> check.
> */
I'll clarify the comment block to show that the NULL check is needed for
non-variable operations, but I prefer to leave the rest the same (i.e. mention
that there is special handling needed - since that is important to mention to
explain why we do not care about the lvalue being NULL in that case).
> > */
> > if (dnp->dn_child->dn_kind == DT_NODE_VAR) {
> > - dt_ident_t *idp = dt_ident_resolve(dnp->dn_child->dn_ident);
> > + idp = dt_ident_resolve(dnp->dn_child->dn_ident);
> > - dt_cg_store_var(dnp, dlp, drp, idp);
> > - } else {
> > - uint_t rbit = dnp->dn_child->dn_flags & DT_NF_REF;
> > + is_dvar = (idp->di_flags & DT_IDFLG_TLS ||
> > + (idp->di_kind == DT_IDENT_ARRAY &&
> > + idp->di_id > DIF_VAR_ARRAY_MAX));
> > + } else
> > + dt_cg_check_notnull(dlp, drp, dnp->dn_child->dn_reg);
> > - assert(dnp->dn_child->dn_flags & DT_NF_WRITABLE);
> > - assert(dnp->dn_child->dn_flags & DT_NF_LVALUE);
> > + if (is_dvar) {
>
> Now, right before the "if (is_dvar)", there can be a comment:
> /*
> * If the lvalue is a dynamic variable, it may not have been created
> yet.
> * The lvalue pointer will be NULL in that case and needs special
> handling.
> */
>
>
> > + dt_node_t val;
> > - dnp->dn_child->dn_flags |= DT_NF_REF; /* force pass-by-ref */
> > - dt_cg_node(dnp->dn_child, dlp, drp);
> > + /*
> > + * The dt_cg_store_var() function expects a dnp->dn_right child
> > + * so we fake one here.
> > + */
> > + val.dn_op = DT_TOK_INT;
> > + val.dn_value = op == BPF_ADD ? adj : -adj;
>
> Why is val.dn_value being set?
Because we want a valid node. ANd because we need it below...
> > - dt_cg_store(dnp, dlp, drp, dnp->dn_child);
> > - dt_regset_free(drp, dnp->dn_child->dn_reg);
> > + lbl_dflt = dt_irlist_label(dlp);
> > + lbl_done = dt_irlist_label(dlp);
> > - dnp->dn_left->dn_flags &= ~DT_NF_REF;
> > - dnp->dn_left->dn_flags |= rbit;
> > - }
> > -}
> > + emit(dlp, BPF_BRANCH_IMM(BPF_JNE, dnp->dn_child->dn_reg, 0, lbl_dflt));
> > + emit(dlp, BPF_MOV_IMM(dnp->dn_reg, val.dn_value));
>
> Instead of val.dn_value, just use op == BPF_ADD ? adj : -adj here. AFAICT,
> val.dn_value need never be set.
But we need it below, and we might as well use a single variable to use for
both cases.
> > -static void
> > -dt_cg_postarith_op(dt_node_t *dnp, dt_irlist_t *dlp,
> > - dt_regset_t *drp, uint_t op)
> > -{
> > - ctf_file_t *ctfp = dnp->dn_ctfp;
> > - ctf_id_t type;
> > - ssize_t size = 1;
> > - int oreg, nreg;
> > + dnp->dn_right = &val;
> > + dt_cg_store_var(dnp, dlp, drp, idp);
And here we do need val to have an actual dn_value.
> > + dnp->dn_right = NULL;
> > + if (post)
> > + emit(dlp, BPF_MOV_IMM(dnp->dn_reg, 0));
> > + emit(dlp, BPF_JUMP(lbl_done));
> > - if (dt_node_is_pointer(dnp)) {
> > - type = ctf_type_resolve(ctfp, dnp->dn_type);
> > - assert(ctf_type_kind(ctfp, type) == CTF_K_POINTER);
> > - size = ctf_type_size(ctfp, ctf_type_reference(ctfp, type));
> > + emitl(dlp, lbl_dflt,
> > + BPF_NOP());
> > }
> > - dt_cg_node(dnp->dn_child, dlp, drp);
> > - dnp->dn_reg = dnp->dn_child->dn_reg;
> > - oreg = dnp->dn_reg;
> > -
> > - nreg = dt_regset_alloc(drp);
> > - if (nreg == -1)
> > - longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> > -
> > - emit(dlp, BPF_MOV_REG(nreg, oreg));
> > - emit(dlp, BPF_ALU64_IMM(op, nreg, size));
> > -
> > - /*
> > - * If we are modifying a variable, generate a store instruction for
> > - * the variable specified by the identifier. If we are storing to a
> > - * memory address, generate code again for the left-hand side using
> > - * DT_NF_REF to get the address, and then generate a store to it.
> > - * In both paths, we store the value from %r0 (the new value).
> > - */
> > - if (dnp->dn_child->dn_kind == DT_NODE_VAR) {
> > - dt_ident_t *idp = dt_ident_resolve(dnp->dn_child->dn_ident);
> > -
> > - dnp->dn_reg = nreg;
> > - dt_cg_store_var(dnp, dlp, drp, idp);
> > - dnp->dn_reg = oreg;
> > + emit(dlp, BPF_LOAD(ldstw[size], dnp->dn_reg, dnp->dn_child->dn_reg, 0));
> > + if (post) {
> > + dt_regset_xalloc(drp, BPF_REG_0);
> > + emit(dlp, BPF_MOV_REG(BPF_REG_0, dnp->dn_reg));
> > + emit(dlp, BPF_ALU64_IMM(op, BPF_REG_0, adj));
> > + emit(dlp, BPF_STORE(ldstw[size], dnp->dn_child->dn_reg, 0, BPF_REG_0));
> > + dt_regset_free(drp, BPF_REG_0);
> > } else {
> > - uint_t rbit = dnp->dn_child->dn_flags & DT_NF_REF;
> > + emit(dlp, BPF_ALU64_IMM(op, dnp->dn_reg, adj));
> > + emit(dlp, BPF_STORE(ldstw[size], dnp->dn_child->dn_reg, 0, dnp->dn_reg));
> > + }
>
> Instead of:
> if (post) {
> dt_regset_xalloc(drp, BPF_REG_0);
> emit(dlp, BPF_MOV_REG(BPF_REG_0, dnp->dn_reg));
> emit(dlp, BPF_ALU64_IMM(op, BPF_REG_0, adj));
> emit(dlp, BPF_STORE(ldstw[size], dnp->dn_child->dn_reg, 0,
> BPF_REG_0));
> dt_regset_free(drp, BPF_REG_0);
> } else {
> emit(dlp, BPF_ALU64_IMM(op, dnp->dn_reg, adj));
> emit(dlp, BPF_STORE(ldstw[size], dnp->dn_child->dn_reg, 0,
> dnp->dn_reg));
> }
>
> how about:
> emit(dlp, BPF_ALU64_IMM(op, BPF_REG_0, adj));
> emit(dlp, BPF_STORE(ldstw[size], dnp->dn_child->dn_reg, 0,
> BPF_REG_0));
> if (post)
> emit(dlp, BPF_ALU64_IMM(op, BPF_REG_0, -adj));
>
> It's simpler, saves register pressure, and somewhat mimics what goes on in
> the "if (is_dvar)" code path. The "extra" arithmetic op doesn't really cost
> anything since trying to save that instruction has its own cost.
I think the original code provides a bit more clarity about the actual
operations.
> > - assert(dnp->dn_child->dn_flags & DT_NF_WRITABLE);
> > - assert(dnp->dn_child->dn_flags & DT_NF_LVALUE);
> > + dt_regset_free(drp, dnp->dn_child->dn_reg);
> > - dnp->dn_child->dn_flags |= DT_NF_REF; /* force pass-by-ref */
> > - dt_cg_node(dnp->dn_child, dlp, drp);
> > -
> > - dnp->dn_reg = nreg;
> > - dt_cg_store(dnp, dlp, drp, dnp->dn_child);
> > - dnp->dn_reg = oreg;
> > + if (is_dvar)
> > + emitl(dlp, lbl_done,
> > + BPF_NOP());
> > - dt_regset_free(drp, dnp->dn_child->dn_reg);
> > - dnp->dn_left->dn_flags &= ~DT_NF_REF;
> > - dnp->dn_left->dn_flags |= rbit;
> > - }
> > + dnp->dn_left->dn_flags &= ~DT_NF_REF;
> > + dnp->dn_left->dn_flags |= rbit;
>
> As noted earlier, turn dn_left into dn_child and restore this bit right
> after the dt_cg_node() call.
>
> > - dt_regset_free(drp, nreg);
> > + TRACE_REGSET(" incdec: End ");
> > }
> > /*
> > @@ -4544,19 +4526,19 @@ dt_cg_node(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> > break;
> > case DT_TOK_PREINC:
> > - dt_cg_prearith_op(dnp, dlp, drp, BPF_ADD);
> > + dt_cg_incdec_op(dnp, dlp, drp, BPF_ADD, 0);
> > break;
> > case DT_TOK_POSTINC:
> > - dt_cg_postarith_op(dnp, dlp, drp, BPF_ADD);
> > + dt_cg_incdec_op(dnp, dlp, drp, BPF_ADD, 1);
> > break;
> > case DT_TOK_PREDEC:
> > - dt_cg_prearith_op(dnp, dlp, drp, BPF_SUB);
> > + dt_cg_incdec_op(dnp, dlp, drp, BPF_SUB, 0);
> > break;
> > case DT_TOK_POSTDEC:
> > - dt_cg_postarith_op(dnp, dlp, drp, BPF_SUB);
> > + dt_cg_incdec_op(dnp, dlp, drp, BPF_SUB, 1);
> > break;
> > case DT_TOK_IPOS:
>
> _______________________________________________
> 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