[DTrace-devel] [PATCH 3/4] clean up dt_cg_typecast(): use single instruction to load shift amount

[Kris Van Hees]

Eugene,

Can you do a patch with the alternative change I mentioned in my earlier reply,
and then it can go in along with the patch to handle the shift by 0 fix?

Thanks,
Kris

On Tue, Aug 11, 2020 at 08:56:42PM -0400, Kris Van Hees wrote:
> On Tue, Aug 11, 2020 at 06:11:11PM -0400, eugene.loh at oracle.com wrote:
> > From: Eugene Loh <eugene.loh at oracle.com>
> > 
> > If a typecast is required, it is effected with a left then right shift.
> > The shift amount is necessarily at most 64.  It is loaded into a
> > register with a two-instruction lddw.  This is unnecessary; a
> > single-instruction IMM move suffices.  This optimization already
> > appears in DT_TOK_INT:
> > 
> >     if (dnp->dn_value > UINT32_MAX)
> >         dt_cg_setx(dlp, dnp->dn_reg, dnp->dn_value);
> >     else {
> >         instr = BPF_MOV_IMM(dnp->dn_reg, dnp->dn_value);
> >         dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> >     }
> > 
> > For the dt_cg_typecast() shift amount, no test is needed;  we already
> > know the shift amount fits in 32 bits.
> 
> This fix is not necessary becausae a different patch needs to be done ere:
> the shift instruction should use an immediate-based shift rather than getting
> the value from a register...
> 
> 	BPF_ALU64_REG(BPF_LSH, dst->dn_reg, reg);
> 
> should be:
> 
> 	BPF_ALU64_IMM(BPF_LSH, dst->dn_reg, n);
> 
> and
> 
> 	BPF_ALU64_REG((dst->dn_flags & DT_NF_SIGNED) ?
> 	              BPF_ARSH : BPF_RSH, dst->dn_reg, reg);
> 
> should be:
> 
> 	BPF_ALU64_IMM((dst->dn_flags & DT_NF_SIGNED) ?
> 	              BPF_ARSH : BPF_RSH, dst->dn_reg, n);
> 
> and the use of 'reg' should be eliminated (no dt_cg_setx or BPF_MOV_IMM
> needed).  This is valid because we know that the shift amount is a constant
> at the moment of generating the code.
> 
> > Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> > ---
> >  libdtrace/dt_cg.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > index 6ba779d1..b3b3d3a7 100644
> > --- a/libdtrace/dt_cg.c
> > +++ b/libdtrace/dt_cg.c
> > @@ -1142,15 +1142,16 @@ dt_cg_typecast(const dt_node_t *src, const dt_node_t *dst,
> >  		if ((reg = dt_regset_alloc(drp)) == -1)
> >  			longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> >  
> > -		dt_cg_setx(dlp, reg, n);
> > +		instr = BPF_MOV_IMM(reg, n);
> > +		dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> >  
> >  		instr = BPF_ALU64_REG(BPF_LSH, dst->dn_reg, reg);
> >  		dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> >  
> >  		instr = BPF_ALU64_REG((dst->dn_flags & DT_NF_SIGNED) ?
> >  		    BPF_ARSH : BPF_RSH, dst->dn_reg, reg);
> > -
> >  		dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> > +
> >  		dt_regset_free(drp, reg);
> >  	}
> >  }
> > -- 
> > 2.18.4
> > 
> > 
> > _______________________________________________
> > DTrace-devel mailing list
> > DTrace-devel at oss.oracle.com
> > https://oss.oracle.com/mailman/listinfo/dtrace-devel
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel