[DTrace-devel] [PATCH 09/10] Improve and unify dt_cg_prearith() and dt_cg_postarith()
Eugene Loh
eugene.loh at oracle.com
Tue Mar 22 19:34:29 UTC 2022
Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
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
> 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.
> 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.
> /*
> - * 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.
*/
> */
> 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?
> - 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.
> -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);
> + 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.
> - 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:
More information about the DTrace-devel
mailing list