Hi!
On Tue, Aug 27, 2019 at 12:08:50PM +0100, Richard Sandiford wrote:
> Mostly trivial formatting comments, but I think there's still a couple
> of substantive points too.
Sorry to piggy-back on your review once again.
> jema...@gnu.org (Jose E. Marchesi) writes:
> > +static rtx
> > +bpf_function_value (const_tree ret_type,
> > + const_tree fntype_or_decl ATTRIBUTE_UNUSED,
>
> This *is* used. :-) I only noticed because...
More modern style doesn't use ATTRIBUTE_UNUSED, it simply names the
parameter unnamed (or commented out, "const_tree /*fntype_or_decl*/").
Good luck accidentally using it, with that C++ style :-)
> > +(define_expand "zero_extendsidi2"
> > + [(set (match_operand:DI 0 "register_operand")
> > + (zero_extend:DI (match_operand:SI 1 "reg_or_indirect_memory_operand")))]
> > + ""
> > +{
> > + if (register_operand (operands[1], SImode))
> > + {
> > + operands[1] = gen_lowpart (DImode, operands[1]);
> > + emit_insn (gen_ashldi3 (operands[0], operands[1], GEN_INT (32)));
> > + emit_insn (gen_lshrdi3 (operands[0], operands[0], GEN_INT (32)));
> > + DONE;
> > + }
> > +})
> > +
> > +(define_insn "*zero_extendsidi2"
> > + [(set (match_operand:DI 0 "register_operand" "=r,r")
> > + (zero_extend:DI (match_operand:SI 1 "reg_or_indirect_memory_operand"
> > "0,m")))]
> > + ""
> > + "@
> > + lsh\t%0,32\n\trsh\t%0,32
> > + ldxw\t%0,%1"
> > + [(set_attr "type" "alu,ldx")
> > + (set_attr "length" "16,8")])
>
> Sorry, should have noticed last time, but: you shouldn't need to handle
> register operands here given the expander above. It's OK if you find it
> improves code quality, but it'd be interesting to know why if so....
Can't you just do "mov32 dst,src" for this? Or "add32 dst,0" if that has
any advantages (maybe it is a shorter insn? No idea).
So that then gets you
(define_insn "zero_extendsidi2"
[(set (match_operand:DI 0 "register_operand" "=r,r")
(zero_extend:DI
(match_operand:SI 1 "reg_or_indirect_memory_operand" "r,m")))]
""
"@
mov32\t%0,%1
ldxw\t%0,%1"
[(set_attr "type" "alu,ldx")])
(and no expander or anything).
(You still need one for the sign-extend, but only a define_expand for that,
nothing more).
Segher
