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:
> + /* Return true if we can use the MIPS32/64 R2 ext/ins instructions.
> +    Return false if OP is not in a register, zero/invalid length bitfields
> +    or lengths equal to word size. */
> +    
> + bool
> + mips_use_ins_ext_p (rtx op, rtx l, rtx p)
> + {

Since you've decided to make mips_use_ins_ext_p take the three
zero_extend arguments separately (rather than pass a pointer as
I suggsted originally ;) you at least need to document all the
arguments.  The comment doesn't mention L and P and doesn't give
much clue what how they relate OP.  Saying that the three arguments
are the three operands to a zero_extend would help.

> !   else if (!mips_use_ins_ext_p (operands[1], operands[2], operands[3]))
>       FAIL;
> +   else
> +     {
> +       if (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;
> +     }
>   })

Since we can't fall through (as I'd hoped at fist) I'd prefer that you
use a positive test here and put the FAIL last.  I.e. swap the bodies
of the two "else"s.

> + /* { dg-final { scan-assembler "ext" } } */
> + /* { dg-final { scan-assembler "ins" } } */
> +
> + struct A 
> + {
> +   unsigned int i : 2;
> +   unsigned int j : 3;
> +   unsigned int k : 4;
> +   unsigned int l : 5;
> + };
> +
> + void func (struct A);
> +
> + unsigned int ext (struct A a)
> + {
> +   return a.j;
> + }
> +
> + void ins (int i)
> + {
> +   struct A c;
> +   c.j = i;
> +   func (c);
> + }

This isn't a very good test.  The output will trivially match "ext"
and "ins" because that's what the functions are called ;)

Patch looks OK otherwise.  The mext patch/review round should be
the last...

Richard