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

[Richard Sandiford <rsandifo at redhat dot com>]

Hi David,

David Ung <davidu@mips.com> writes:
> This patch adds patterns for generating the mips32 release 2 ext/inv
> insn to mips.md.
>
> ok to commit??

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.

> +   else if (!ISA_MIPS32R2
> + 	   || !register_operand (operands[1], VOIDmode))
> +     FAIL;
>     else
> +     {
> +       /* MIPS ins/ext insns don't support zero length bitfields or fields
> +          extending beyond the left or right-most bits.  Also, we reject 
> + 	 lengths equal to a word as they are better handled by the move 
> + 	 patterns.  */
> + 
> +       HOST_WIDE_INT len = INTVAL (operands[2]);  
> +       HOST_WIDE_INT pos = INTVAL (operands[3]); 
> +       
> +       if (len <= 0 || len >= BITS_PER_WORD || pos < 0 
> + 	  || pos + len > BITS_PER_WORD)    
>           FAIL;
> + 

Please don't use ISA_MIPS32R2.  Define an ISA_HAS_* macro instead.
Also, this code has been cut-&-pasted in both the extzv and insv
expanders.  Please put it in a helper function so that both expanders
can use it.  The function could take a pointer to the three zero_extract
operands and verify (a) that the ISA has the instructions and (b) that
the operands are valid.

> +       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.

In summary, I think you can simply add:

    else if (!mips_use_ins_ext_p (&operands[1]))
      FAIL;

to the end of the existing extzv pattern, and similarly:

    else if (!mips_use_ins_ext_p (&operands[0]))
      FAIL;

for the insertion expander.

> + (define_insn "extzvsi"
> +   [(set (match_operand:SI 0 "register_operand" "=d")
> + 	(zero_extract:SI (match_operand:SI 1 "register_operand" "d")
> + 			 (match_operand:SI 2 "immediate_operand" "I")
> + 			 (match_operand:SI 3 "immediate_operand" "I")))]
> +   "ISA_MIPS32R2 && !TARGET_MIPS16"
> +   "*
> + {
> +   operands[2] = GEN_INT (INTVAL (operands[2]) & 31);
> +   operands[3] = GEN_INT (INTVAL (operands[3]) & 31);
> +   return \"ext\\t%0,%1,%3,%2\";
> + }")
> + 
> + (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.  I suppose it might be possible for an rtl optimisation pass to
generate different zero_extracts later, in which case it would probably
make sense to add a "mips_use_ins_ext_p" check to the define_insns too:

    (define_insn "extzv<mode>"
      [(...)]
      "mips_use_ins_ext_p (&operands[1])"

Same comments apply to the insertion patterns.

Richard