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

[Eugene Loh]

On 1/8/24 19:37, Kris Van Hees wrote:

> 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>
>>
>> On 1/5/24 00:29, Kris Van Hees via DTrace-devel wrote:
>>> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
>>> @@ -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.

Okay.  A comment in the patch would be great.  Otherwise, the complexity 
makes no sense.