Re: [patch] MIPS: add mips32r2 patterns to generate ext and ins insns.

[Richard Sandiford <rsandifo at redhat dot com>]

David Ung <davidu@mips.com> writes:
>> Please remember to say how you tested patches.  You need to do a
>> regression test for some configuration (one that would test your
>> changes, of course. ;)  You should also add testcases to
>> gcc.target/mips to make sure that these new instructions
>> are being used in the right circumstances.
>
> right.  I tested with mips-sim-idt32//mips32r2 (with the new ext/ins
> added to the GNU simulator), it had no new gcc regressions.
> I'll see if I can add some testcases aswell.

Thanks.

>> > +       if (TARGET_64BIT && GET_MODE (operands[0]) == DImode)
>> > +         emit_insn (gen_extzvdi (operands[0], operands[1], operands[2], 
>> > + 				operands[3]));
>> > +       else
>> > +         emit_insn (gen_extzvsi (operands[0], operands[1], operands[2], 
>> > +                                 operands[3]));
>> > +
>> > +       DONE;
>> 
>> Why do you need to do this?  I'd have thought the expander would generate
>> the right pattern if you just fell through.
>
> note the template modes for extzv, extzvsi and extzvdi.
> The expander don't have any modes, while the other 2 have :SI and :DI
> attach to them.  the emit_insn (gen_extzv.. forces the patterns to
> generate in the right form with the correct modes.  If I remove the
> above, it would generate something like
>
> (insn 9 8 10 1 (set (subreg:SI (reg:HI 186) 0)
>         (zero_extract (reg/v:SI 185 [ x ])
>             (const_int 10 [0xa])
>             (const_int 15 [0xf]))) -1 (nil)
>     (nil))

Ah, good point.  I'd missed that.

> I can't add the mode macro to expander like 
>
> (define_expand "extzv"
>   [(set (match_operand:GPR 0 "register_operand")
> 	(zero_extract:GPR (match_operand:GPR 1 "nonimmediate_operand")
> 		      (match_operand:GPR 2 "immediate_operand")
> 		      (match_operand:GPR 3 "immediate_operand")))]
>
> because there's only one pattern for extzv.  :(

Yeah.  I guess there really is no getting around it.  Oh well.

Please remove the "TARGET_64BIT" check above though.  We should only
get here for DImode if a DImode pattern is valid.

>> > + 
>> > + (define_insn "extzvdi"
>> > +   [(set (match_operand:DI 0 "register_operand" "=d")
>> > + 	(zero_extract:DI (match_operand:DI 1 "register_operand" "d")
>> > + 			 (match_operand:SI 2 "immediate_operand" "I")
>> > + 			 (match_operand:SI 3 "immediate_operand" "I")))]
>> > +   "ISA_MIPS32R2 && TARGET_64BIT && !TARGET_MIPS16"
>> > +   "*
>> > + {
>> > +   operands[2] = GEN_INT (INTVAL (operands[2]) & 63);
>> > +   operands[3] = GEN_INT (INTVAL (operands[3]) & 63);
>> > +   return \"dext\\t%0,%1,%3,%2\";
>> > + }")
>> 
>> Please use :GPR to combine these patterns.  And are the masks actually
>> necessary?  It seems a bit worrying if so.  The "pos" and "len" checks
>> in your expander should make sure that the bitranges do not need to be
>> masked.  
>
> yes.  These masks may well be redundant now with the checks in the
> expander.  I'll do some tests.

Thanks.

Richard