diff mbox

[3/3] Record the number of generator arguments in insn_data

Message ID g4aagbl0nm.fsf@linaro.org
State Accepted
Headers show

Commit Message

Richard Sandiford March 31, 2011, 4:23 p.m. UTC 
This patch makes use of the new n_generator_args field.

Bboostrapped & regressions-tested on x86_64-linux-gnu.  Also tested by
building cc1 for arm-linux-gnueabi, mips-elf, spu-elf.

Richard


gcc/
	* expr.c (emit_block_move_via_movmem): Use n_generator_args
	instead of n_operands.
	* optabs.c (maybe_gen_insn): Likewise.
	* config/arm/arm.c (arm_init_neon_builtins): Likewise.
	* config/mips/mips.c (mips_expand_builtin_compare_1): Likewise.
	(mips_expand_builtin_direct): Likewise.
	* config/spu/spu.c (expand_builtin_args): Likewise.

Comments

Richard Henderson March 31, 2011, 7:47 p.m. UTC | #1 
On 03/31/2011 09:23 AM, Richard Sandiford wrote:
> +++ gcc/expr.c	2011-03-31 16:49:06.000000000 +0100
> @@ -1293,11 +1293,7 @@ emit_block_move_via_movmem (rtx x, rtx y
>  	     nice if there were some way to inform the backend, so
>  	     that it doesn't fail the expansion because it thinks
>  	     emitting the libcall would be more efficient.  */
> -	  nops = insn_data[(int) code].n_operands;
> -	  /* ??? n_operands includes match_scratches; find some other
> -	     way to select the 6 operand variant, or force all targets
> -	     to have exactly 6 operands.  */
> -	  gcc_assert (nops >= 4 && nops <= 6);
> +	  nops = insn_data[(int) code].n_generator_args;

I think the assert should be retained (for now) as

  gcc_assert (nops == 4 || nops == 6);

at least until we add such verification to some genfoo.

Also, you've forgotten the setmem hunk.


r~
Richard Sandiford March 31, 2011, 8:09 p.m. UTC | #2 
Richard Henderson <rth@redhat.com> writes:
> On 03/31/2011 09:23 AM, Richard Sandiford wrote:
>> +++ gcc/expr.c	2011-03-31 16:49:06.000000000 +0100
>> @@ -1293,11 +1293,7 @@ emit_block_move_via_movmem (rtx x, rtx y
>>  	     nice if there were some way to inform the backend, so
>>  	     that it doesn't fail the expansion because it thinks
>>  	     emitting the libcall would be more efficient.  */
>> -	  nops = insn_data[(int) code].n_operands;
>> -	  /* ??? n_operands includes match_scratches; find some other
>> -	     way to select the 6 operand variant, or force all targets
>> -	     to have exactly 6 operands.  */
>> -	  gcc_assert (nops >= 4 && nops <= 6);
>> +	  nops = insn_data[(int) code].n_generator_args;
>
> I think the assert should be retained (for now) as
>
>   gcc_assert (nops == 4 || nops == 6);
>
> at least until we add such verification to some genfoo.

After the patch we have:

  gcc_assert (nops == (unsigned int) insn_data[(int) icode].n_generator_args);

in maybe_gen_insn, which should catch this.  Just to check: do you want
the assertion here anyway?

> Also, you've forgotten the setmem hunk.

Gah.  The movmem and setmem bits look so similar that I forgot there
there two.

Richard
Richard Henderson March 31, 2011, 8:12 p.m. UTC | #3 
On 03/31/2011 01:09 PM, Richard Sandiford wrote:
>> I think the assert should be retained (for now) as
>>
>>   gcc_assert (nops == 4 || nops == 6);
>>
>> at least until we add such verification to some genfoo.
> 
> After the patch we have:
> 
>   gcc_assert (nops == (unsigned int) insn_data[(int) icode].n_generator_args);
> 
> in maybe_gen_insn, which should catch this.  Just to check: do you want
> the assertion here anyway?

Well, it won't because we initialized nops *with .n_generator_args.

If nops was initialized with

  nops = 4;
  create the first 4 args

  if (.n_generator_args == 6)
    {
      nops = 6;
      create the last two args
    }

then the assert you mention *would* do the trick.


r~
diff mbox

Patch

Index: gcc/expr.c
===================================================================
--- gcc/expr.c	2011-03-31 16:40:14.000000000 +0100
+++ gcc/expr.c	2011-03-31 16:49:06.000000000 +0100
@@ -1293,11 +1293,7 @@  emit_block_move_via_movmem (rtx x, rtx y
 	     nice if there were some way to inform the backend, so
 	     that it doesn't fail the expansion because it thinks
 	     emitting the libcall would be more efficient.  */
-	  nops = insn_data[(int) code].n_operands;
-	  /* ??? n_operands includes match_scratches; find some other
-	     way to select the 6 operand variant, or force all targets
-	     to have exactly 6 operands.  */
-	  gcc_assert (nops >= 4 && nops <= 6);
+	  nops = insn_data[(int) code].n_generator_args;
 
 	  create_fixed_operand (&ops[0], x);
 	  create_fixed_operand (&ops[1], y);
Index: gcc/optabs.c
===================================================================
--- gcc/optabs.c	2011-03-31 16:40:14.000000000 +0100
+++ gcc/optabs.c	2011-03-31 16:49:06.000000000 +0100
@@ -7149,9 +7149,7 @@  maybe_legitimize_operands (enum insn_cod
 maybe_gen_insn (enum insn_code icode, unsigned int nops,
 		struct expand_operand *ops)
 {
-  /* n_operands includes any automatically-generated match_scratches,
-     so we can't check for equality here.  */
-  gcc_assert (nops <= (unsigned int) insn_data[(int) icode].n_operands);
+  gcc_assert (nops == (unsigned int) insn_data[(int) icode].n_generator_args);
   if (!maybe_legitimize_operands (icode, 0, nops, ops))
     return NULL_RTX;
 
Index: gcc/config/arm/arm.c
===================================================================
--- gcc/config/arm/arm.c	2011-03-31 16:40:14.000000000 +0100
+++ gcc/config/arm/arm.c	2011-03-31 16:49:06.000000000 +0100
@@ -18944,7 +18944,7 @@  arm_init_neon_builtins (void)
 		/* Build a function type directly from the insn_data for this
 		   builtin.  The build_function_type() function takes care of
 		   removing duplicates for us.  */
-		for (k = insn_data[icode].n_operands - 1; k >= 0; k--)
+		for (k = insn_data[icode].n_generator_args - 1; k >= 0; k--)
 		  {
 		    tree eltype;
 
Index: gcc/config/mips/mips.c
===================================================================
--- gcc/config/mips/mips.c	2011-03-31 16:40:14.000000000 +0100
+++ gcc/config/mips/mips.c	2011-03-31 16:49:06.000000000 +0100
@@ -13252,7 +13252,7 @@  mips_expand_builtin_compare_1 (enum insn
 
   /* The instruction should have a target operand, an operand for each
      argument, and an operand for COND.  */
-  gcc_assert (nargs + 2 == insn_data[(int) icode].n_operands);
+  gcc_assert (nargs + 2 == insn_data[(int) icode].n_generator_args);
 
   opno = 0;
   create_output_operand (&ops[opno++], NULL_RTX,
@@ -13280,11 +13280,9 @@  mips_expand_builtin_direct (enum insn_co
   if (has_target_p)
     create_output_operand (&ops[opno++], target, TYPE_MODE (TREE_TYPE (exp)));
 
-  /* Map the arguments to the other operands.  The n_operands value
-     for an expander includes match_dups and match_scratches as well as
-     match_operands, so n_operands is only an upper bound on the number
-     of arguments to the expander function.  */
-  gcc_assert (opno + call_expr_nargs (exp) <= insn_data[icode].n_operands);
+  /* Map the arguments to the other operands.  */
+  gcc_assert (opno + call_expr_nargs (exp)
+	      == insn_data[icode].n_generator_args);
   for (argno = 0; argno < call_expr_nargs (exp); argno++)
     mips_prepare_builtin_arg (&ops[opno++], exp, argno);
 
Index: gcc/config/spu/spu.c
===================================================================
--- gcc/config/spu/spu.c	2011-03-31 16:40:14.000000000 +0100
+++ gcc/config/spu/spu.c	2011-03-31 16:49:06.000000000 +0100
@@ -6545,9 +6545,7 @@  expand_builtin_args (struct spu_builtin_
       ops[i] = expand_expr (arg, NULL_RTX, VOIDmode, EXPAND_NORMAL);
     }
 
-  /* The insn pattern may have additional operands (SCRATCH).
-     Return the number of actual non-SCRATCH operands.  */
-  gcc_assert (i <= insn_data[icode].n_operands);
+  gcc_assert (i == insn_data[icode].n_generator_args);
   return i;
 }