RFA: Tweak optabs.c's use of constant rtx_costs

RFA: Tweak optabs.c's use of constant rtx_costs

[Richard Sandiford]

expand_binop has code like the following:

  /* If we are inside an appropriately-short loop and we are optimizing,
     force expensive constants into a register.  */
  if (CONSTANT_P (op0) && optimize
      && rtx_cost (op0, binoptab->code) > COSTS_N_INSNS (1))
    {
      if (GET_MODE (op0) != VOIDmode)
	op0 = convert_modes (mode, VOIDmode, op0, unsignedp);
      op0 = force_reg (mode, op0);
    }

Outdated comment aside, what this code is doing seems perfectly
reasonable in itself.  However, I think it's in the wrong place.
The code is one of the first things that expand_binop does,
so we run it even if we end up splitting the binary operation
into smaller word-sized pieces or using a sequence of completely
different optabs altogether.  There are two direct problems with this:

  - The target has no real way of knowing whether a CONST_INT passed
    to TARGET_RTX_COSTS is for a word or double-word operation.

  - If we force a constant into a register before reducing the operation
    into smaller pieces (such as word-mode pieces), we will end up using
    SUBREGs of a multiword REG for those smaller pieces, instead of using
    individual constants.  That's harder to optimise, and effectively
    hides the individual constants until after the lower-subreg pass.

This patch therefore moves the force-to-reg checks to two places:

  - expand_binop_directly, before using an optab.

  - The part of expand_binop that widens operands from mode M1 to
    mode M2, if we don't care about the high bits of the widened
    operand or result.  In this case we force the M1-mode constant
    into a register and then generate an M2-mode paradoxical subreg.
    (This is what we do now too, and is important for targets like ARM,
    where we can load more HImode constants than we can their
    sign-extended SImode equivalents.)

It also moves the commutative operand canonicalisation so that
it continues to happen after forcing constants to registers.

I have some mips_rtx_costs tweaks that make things better with
this patch but often makes things worse without it.

Bootstrapped & regression-tested on x86_64-linux-gnu.  I ran CSiBE
for arm-elf, sh-elf and x86_64-linux-gnu, all with -Os.  There were
no changes for x86, a slight improvement for ARM:

jpeg:jquant1                                      3704     3700 :   99.89%
teem:src/nrrd/tmfKernel                         202825   202781 :   99.98%
teem:src/echo/test/trend                         13609    12845 :   94.39%
----------------------------------------------------------------
Total                                          3633455  3632643 :   99.98%

and another slight improvement for SH:

linux:fs/ext3/balloc                              4108     4100 :   99.81%
linux:arch/testplatform/kernel/signal             4008     3980 :   99.30%
linux:arch/testplatform/kernel/traps              7988     7928 :   99.25%
teem:src/nrrd/kernel                             27716    27704 :   99.96%
teem:src/nrrd/endianNrrd                           688      648 :   94.19%
teem:src/nrrd/tmfKernel                         199876   199936 :  100.03%
teem:src/limn/test/tcamanim                       3624     3620 :   99.89%
teem:src/air/754                                  1964     1960 :   99.80%
teem:src/echo/test/trend                         10300     8056 :   78.21%
unrarlib:unrarlib/unrarlib                       12124    12140 :  100.13%
----------------------------------------------------------------
Total                                          3183600  3181276 :   99.93%

OK to install?

Richard


gcc/
	* optabs.c (shift_optab_p, commutative_optab_p): New functions,
	split out from expand_binop.
	(avoid_expensive_constant): New function.
	(expand_binop_directly): Remove commutative_op argument and
	call cummutative_optab_p instead.  Do not change op0 or op1
	when swapping xop0 and xop1.  Apply avoid_expensive_constant
	to each argument after potential swapping.  Enforce the
	canonical order of commutative operands.
	(expand_binop): Use shift_optab_p and commutative_optab_p.
	Update the calls to expand_binop_directly.  Only force constants
	into registers when widening an operation.  Only swap operands
	once a direct expansion has been rejected.
	(expand_twoval_binop): Only force constants into registers when
	using a direct expansion.

Index: gcc/optabs.c
===================================================================
--- gcc/optabs.c	2007-08-17 14:05:14.000000000 +0100
+++ gcc/optabs.c	2007-08-17 14:24:49.000000000 +0100
@@ -1246,6 +1246,56 @@ swap_commutative_operands_with_target (r
     return rtx_equal_p (op1, target);
 }
 
+/* Return true if BINOPTAB implements a shift operation.  */
+
+static bool
+shift_optab_p (optab binoptab)
+{
+  switch (binoptab->code)
+    {
+    case ASHIFT:
+    case ASHIFTRT:
+    case LSHIFTRT:
+    case ROTATE:
+    case ROTATERT:
+      return true;
+
+    default:
+      return false;
+    }
+}
+
+/* Return true if BINOPTAB implements a commutatative binary operation.  */
+
+static bool
+commutative_optab_p (optab binoptab)
+{
+  return (GET_RTX_CLASS (binoptab->code) == RTX_COMM_ARITH
+	  || binoptab == smul_widen_optab
+	  || binoptab == umul_widen_optab
+	  || binoptab == smul_highpart_optab
+	  || binoptab == umul_highpart_optab);
+}
+
+/* X is to be used in mode MODE as an operand to BINOPTAB.  If we're
+   optimizing, and if the operand is a constant that costs more than
+   1 instruction, force the constant into a register and return that
+   register.  Return X otherwise.  UNSIGNEDP says whether X is unsigned.  */
+
+static rtx
+avoid_expensive_constant (enum machine_mode mode, optab binoptab,
+			  rtx x, bool unsignedp)
+{
+  if (optimize
+      && CONSTANT_P (x)
+      && rtx_cost (x, binoptab->code) > COSTS_N_INSNS (1))
+    {
+      if (GET_MODE (x) != VOIDmode)
+	x = convert_modes (mode, VOIDmode, x, unsignedp);
+      x = force_reg (mode, x);
+    }
+  return x;
+}
 
 /* Helper function for expand_binop: handle the case where there
    is an insn that directly implements the indicated operation.
@@ -1254,55 +1304,72 @@ swap_commutative_operands_with_target (r
 expand_binop_directly (enum machine_mode mode, optab binoptab,
 		       rtx op0, rtx op1,
 		       rtx target, int unsignedp, enum optab_methods methods,
-		       int commutative_op, rtx last)
+		       rtx last)
 {
   int icode = (int) optab_handler (binoptab, mode)->insn_code;
   enum machine_mode mode0 = insn_data[icode].operand[1].mode;
   enum machine_mode mode1 = insn_data[icode].operand[2].mode;
   enum machine_mode tmp_mode;
+  bool commutative_p;
   rtx pat;
   rtx xop0 = op0, xop1 = op1;
   rtx temp;
+  rtx swap;
   
   if (target)
     temp = target;
   else
     temp = gen_reg_rtx (mode);
-  
+
   /* If it is a commutative operator and the modes would match
      if we would swap the operands, we can save the conversions.  */
-  if (commutative_op)
-    {
-      if (GET_MODE (op0) != mode0 && GET_MODE (op1) != mode1
-	  && GET_MODE (op0) == mode1 && GET_MODE (op1) == mode0)
-	{
-	  rtx tmp;
-	  
-	  tmp = op0; op0 = op1; op1 = tmp;
-	  tmp = xop0; xop0 = xop1; xop1 = tmp;
-	}
+  commutative_p = commutative_optab_p (binoptab);
+  if (commutative_p
+      && GET_MODE (xop0) != mode0 && GET_MODE (xop1) != mode1
+      && GET_MODE (xop0) == mode1 && GET_MODE (xop1) == mode1)
+    {
+      swap = xop0;
+      xop0 = xop1;
+      xop1 = swap;
     }
   
+  /* If we are optimizing, force expensive constants into a register.  */
+  xop0 = avoid_expensive_constant (mode0, binoptab, xop0, unsignedp);
+  if (!shift_optab_p (binoptab))
+    xop1 = avoid_expensive_constant (mode1, binoptab, xop1, unsignedp);
+
   /* In case the insn wants input operands in modes different from
      those of the actual operands, convert the operands.  It would
      seem that we don't need to convert CONST_INTs, but we do, so
      that they're properly zero-extended, sign-extended or truncated
      for their mode.  */
   
-  if (GET_MODE (op0) != mode0 && mode0 != VOIDmode)
+  if (GET_MODE (xop0) != mode0 && mode0 != VOIDmode)
     xop0 = convert_modes (mode0,
-			  GET_MODE (op0) != VOIDmode
-			  ? GET_MODE (op0)
+			  GET_MODE (xop0) != VOIDmode
+			  ? GET_MODE (xop0)
 			  : mode,
 			  xop0, unsignedp);
   
-  if (GET_MODE (op1) != mode1 && mode1 != VOIDmode)
+  if (GET_MODE (xop1) != mode1 && mode1 != VOIDmode)
     xop1 = convert_modes (mode1,
-			  GET_MODE (op1) != VOIDmode
-			  ? GET_MODE (op1)
+			  GET_MODE (xop1) != VOIDmode
+			  ? GET_MODE (xop1)
 			  : mode,
 			  xop1, unsignedp);
   
+  /* If operation is commutative,
+     try to make the first operand a register.
+     Even better, try to make it the same as the target.
+     Also try to make the last operand a constant.  */
+  if (commutative_p
+      && swap_commutative_operands_with_target (target, xop0, xop1))
+    {
+      swap = xop1;
+      xop1 = xop0;
+      xop0 = swap;
+    }
+
   /* Now, if insn's predicates don't allow our operands, put them into
      pseudo regs.  */
   
@@ -1375,12 +1442,6 @@ expand_binop (enum machine_mode mode, op
   enum mode_class class;
   enum machine_mode wider_mode;
   rtx temp;
-  int commutative_op = 0;
-  int shift_op = (binoptab->code == ASHIFT
-		  || binoptab->code == ASHIFTRT
-		  || binoptab->code == LSHIFTRT
-		  || binoptab->code == ROTATE
-		  || binoptab->code == ROTATERT);
   rtx entry_last = get_last_insn ();
   rtx last;
 
@@ -1395,54 +1456,16 @@ expand_binop (enum machine_mode mode, op
       binoptab = add_optab;
     }
 
-  /* If we are inside an appropriately-short loop and we are optimizing,
-     force expensive constants into a register.  */
-  if (CONSTANT_P (op0) && optimize
-      && rtx_cost (op0, binoptab->code) > COSTS_N_INSNS (1))
-    {
-      if (GET_MODE (op0) != VOIDmode)
-	op0 = convert_modes (mode, VOIDmode, op0, unsignedp);
-      op0 = force_reg (mode, op0);
-    }
-
-  if (CONSTANT_P (op1) && optimize
-      && ! shift_op && rtx_cost (op1, binoptab->code) > COSTS_N_INSNS (1))
-    {
-      if (GET_MODE (op1) != VOIDmode)
-	op1 = convert_modes (mode, VOIDmode, op1, unsignedp);
-      op1 = force_reg (mode, op1);
-    }
-
   /* Record where to delete back to if we backtrack.  */
   last = get_last_insn ();
 
-  /* If operation is commutative,
-     try to make the first operand a register.
-     Even better, try to make it the same as the target.
-     Also try to make the last operand a constant.  */
-  if (GET_RTX_CLASS (binoptab->code) == RTX_COMM_ARITH
-      || binoptab == smul_widen_optab
-      || binoptab == umul_widen_optab
-      || binoptab == smul_highpart_optab
-      || binoptab == umul_highpart_optab)
-    {
-      commutative_op = 1;
-
-      if (swap_commutative_operands_with_target (target, op0, op1))
-	{
-	  temp = op1;
-	  op1 = op0;
-	  op0 = temp;
-	}
-    }
-
   /* If we can do it with a three-operand insn, do so.  */
 
   if (methods != OPTAB_MUST_WIDEN
       && optab_handler (binoptab, mode)->insn_code != CODE_FOR_nothing)
     {
       temp = expand_binop_directly (mode, binoptab, op0, op1, target,
-				    unsignedp, methods, commutative_op, last);
+				    unsignedp, methods, last);
       if (temp)
 	return temp;
     }
@@ -1469,8 +1492,7 @@ expand_binop (enum machine_mode mode, op
 			       NULL_RTX, unsignedp, OPTAB_DIRECT);
 				   
       temp = expand_binop_directly (mode, otheroptab, op0, newop1,
-				    target, unsignedp, methods,
-				    commutative_op, last);
+				    target, unsignedp, methods, last);
       if (temp)
 	return temp;
     }
@@ -1529,7 +1551,14 @@ expand_binop (enum machine_mode mode, op
 		 || binoptab == add_optab || binoptab == sub_optab
 		 || binoptab == smul_optab || binoptab == ashl_optab)
 		&& class == MODE_INT)
-	      no_extend = 1;
+	      {
+		no_extend = 1;
+		xop0 = avoid_expensive_constant (mode, binoptab,
+						 xop0, unsignedp);
+		if (binoptab != ashl_optab)
+		  xop1 = avoid_expensive_constant (mode, binoptab,
+						   xop1, unsignedp);
+	      }
 
 	    xop0 = widen_operand (xop0, wider_mode, mode, unsignedp, no_extend);
 
@@ -1558,6 +1587,18 @@ expand_binop (enum machine_mode mode, op
 	  }
       }
 
+  /* If operation is commutative,
+     try to make the first operand a register.
+     Even better, try to make it the same as the target.
+     Also try to make the last operand a constant.  */
+  if (commutative_optab_p (binoptab)
+      && swap_commutative_operands_with_target (target, op0, op1))
+    {
+      temp = op1;
+      op1 = op0;
+      op0 = temp;
+    }
+
   /* These can be done a word at a time.  */
   if ((binoptab == and_optab || binoptab == ior_optab || binoptab == xor_optab)
       && class == MODE_INT
@@ -1980,7 +2021,7 @@ expand_binop (enum machine_mode mode, op
 
       start_sequence ();
 
-      if (shift_op)
+      if (shift_optab_p (binoptab))
 	{
 	  op1_mode = targetm.libgcc_shift_count_mode ();
 	  /* Specify unsigned here,
@@ -2256,16 +2297,6 @@ expand_twoval_binop (optab binoptab, rtx
 
   class = GET_MODE_CLASS (mode);
 
-  /* If we are inside an appropriately-short loop and we are optimizing,
-     force expensive constants into a register.  */
-  if (CONSTANT_P (op0) && optimize
-      && rtx_cost (op0, binoptab->code) > COSTS_N_INSNS (1))
-    op0 = force_reg (mode, op0);
-
-  if (CONSTANT_P (op1) && optimize
-      && rtx_cost (op1, binoptab->code) > COSTS_N_INSNS (1))
-    op1 = force_reg (mode, op1);
-
   if (!targ0)
     targ0 = gen_reg_rtx (mode);
   if (!targ1)
@@ -2282,6 +2313,10 @@ expand_twoval_binop (optab binoptab, rtx
       rtx pat;
       rtx xop0 = op0, xop1 = op1;
 
+      /* If we are optimizing, force expensive constants into a register.  */
+      xop0 = avoid_expensive_constant (mode0, binoptab, xop0, unsignedp);
+      xop1 = avoid_expensive_constant (mode1, binoptab, xop1, unsignedp);
+
       /* In case the insn wants input operands in modes different from
 	 those of the actual operands, convert the operands.  It would
 	 seem that we don't need to convert CONST_INTs, but we do, so

Re: RFA: Tweak optabs.c's use of constant rtx_costs

[Rask Ingemann Lambertsen]

On Fri, Aug 17, 2007 at 04:28:18PM +0100, Richard Sandiford wrote:
> expand_binop has code like the following:
> 
>   /* If we are inside an appropriately-short loop and we are optimizing,
>      force expensive constants into a register.  */
>   if (CONSTANT_P (op0) && optimize
>       && rtx_cost (op0, binoptab->code) > COSTS_N_INSNS (1))
>     {
>       if (GET_MODE (op0) != VOIDmode)
> 	op0 = convert_modes (mode, VOIDmode, op0, unsignedp);
>       op0 = force_reg (mode, op0);
>     }
> 
> Outdated comment aside, what this code is doing seems perfectly
> reasonable in itself.  However, I think it's in the wrong place.
> The code is one of the first things that expand_binop does,
> so we run it even if we end up splitting the binary operation
> into smaller word-sized pieces or using a sequence of completely
> different optabs altogether.  There are two direct problems with this:
> 
>   - The target has no real way of knowing whether a CONST_INT passed
>     to TARGET_RTX_COSTS is for a word or double-word operation.
> 
>   - If we force a constant into a register before reducing the operation
>     into smaller pieces (such as word-mode pieces), we will end up using
>     SUBREGs of a multiword REG for those smaller pieces, instead of using
>     individual constants.  That's harder to optimise, and effectively
>     hides the individual constants until after the lower-subreg pass.

   There is similiar code in prepare_cmp_insn(). It interferes with
SELECT_CC_MODE() on targets which choose the mode of the comparison result
depending on the constant (if any) in the comparison.

   Additionally, emitting

(insn (set (tmp) (const_int 4)))
(insn (set (sp) (plus (sp) (tmp))))

instead of

(insn (set (sp) (plus (sp) (const_int 4))))

disables frame pointer elimination for a function. Fwprop, gcse and
sometimes even combine can clean it up, but that of course requires those
passes to be run. It's a nuisance when trying to find a code gen bug which
only happens with frame pointer elimination.

   To have the magic constant COSTS_N_INSNS (1) hard coded into a target
independent piece of the compiler isn't nice. It is an improvment to have
fewer such places.

-- 
Rask Ingemann Lambertsen

Re: RFA: Tweak optabs.c's use of constant rtx_costs

[Richard Sandiford]

Rask Ingemann Lambertsen <rask@sygehus.dk> writes:
> On Fri, Aug 17, 2007 at 04:28:18PM +0100, Richard Sandiford wrote:
>> expand_binop has code like the following:
>> 
>>   /* If we are inside an appropriately-short loop and we are optimizing,
>>      force expensive constants into a register.  */
>>   if (CONSTANT_P (op0) && optimize
>>       && rtx_cost (op0, binoptab->code) > COSTS_N_INSNS (1))
>>     {
>>       if (GET_MODE (op0) != VOIDmode)
>> 	op0 = convert_modes (mode, VOIDmode, op0, unsignedp);
>>       op0 = force_reg (mode, op0);
>>     }
>> 
>> Outdated comment aside, what this code is doing seems perfectly
>> reasonable in itself.  However, I think it's in the wrong place.
>> The code is one of the first things that expand_binop does,
>> so we run it even if we end up splitting the binary operation
>> into smaller word-sized pieces or using a sequence of completely
>> different optabs altogether.  There are two direct problems with this:
>> 
>>   - The target has no real way of knowing whether a CONST_INT passed
>>     to TARGET_RTX_COSTS is for a word or double-word operation.
>> 
>>   - If we force a constant into a register before reducing the operation
>>     into smaller pieces (such as word-mode pieces), we will end up using
>>     SUBREGs of a multiword REG for those smaller pieces, instead of using
>>     individual constants.  That's harder to optimise, and effectively
>>     hides the individual constants until after the lower-subreg pass.
>
>    There is similiar code in prepare_cmp_insn(). It interferes with
> SELECT_CC_MODE() on targets which choose the mode of the comparison result
> depending on the constant (if any) in the comparison.
>
>    Additionally, emitting
>
> (insn (set (tmp) (const_int 4)))
> (insn (set (sp) (plus (sp) (tmp))))
>
> instead of
>
> (insn (set (sp) (plus (sp) (const_int 4))))
>
> disables frame pointer elimination for a function. Fwprop, gcse and
> sometimes even combine can clean it up, but that of course requires those
> passes to be run. It's a nuisance when trying to find a code gen bug which
> only happens with frame pointer elimination.

Yeah, I'd noticed the prepare_cmp_insn insn thing too; it means that
the MIPS patch I mentioned has to treat constants with a COMPARE outer
code specially -- giving them zero cost -- even though the port never
actually uses COMPARE itself.  I didn't actually find the MIPS check
too gross, but I did wonder if the code was still worthwhile on targets
that do use COMPARE.  It's interesting that it's a pain for yours too.

(For avoidance of doubt, I think this should be treated as a separate
issue, and I don't think you were suggesting otherwise.)

Richard

Re: RFA: Tweak optabs.c's use of constant rtx_costs

[Rask Ingemann Lambertsen]

On Fri, Aug 17, 2007 at 06:05:44PM +0100, Richard Sandiford wrote:
> Rask Ingemann Lambertsen <rask@sygehus.dk> writes:
> > On Fri, Aug 17, 2007 at 04:28:18PM +0100, Richard Sandiford wrote:
> >> expand_binop has code like the following:
> >> 
> >>   /* If we are inside an appropriately-short loop and we are optimizing,
> >>      force expensive constants into a register.  */
> >>   if (CONSTANT_P (op0) && optimize
> >>       && rtx_cost (op0, binoptab->code) > COSTS_N_INSNS (1))
> >>     {
> >>       if (GET_MODE (op0) != VOIDmode)
> >> 	op0 = convert_modes (mode, VOIDmode, op0, unsignedp);
> >>       op0 = force_reg (mode, op0);
> >>     }
> >
> >    There is similiar code in prepare_cmp_insn(). It interferes with
> > SELECT_CC_MODE() on targets which choose the mode of the comparison result
> > depending on the constant (if any) in the comparison.
> 
> Yeah, I'd noticed the prepare_cmp_insn insn thing too; it means that
> the MIPS patch I mentioned has to treat constants with a COMPARE outer
> code specially -- giving them zero cost -- even though the port never
> actually uses COMPARE itself.  I didn't actually find the MIPS check
> too gross, but I did wonder if the code was still worthwhile on targets
> that do use COMPARE.  It's interesting that it's a pain for yours too.

   I've updated the ia16 target I posted to use "lazy" comparison like many
other targets do; initially it would miss some optimizations because of
this. I now have a few extra lines in ia16_rtx_costs() to work around it:

      /* Take counter measures against prepare_cmp_insn().  */
      if (outer_code == COMPARE && *total > COSTS_N_INSNS (1))
        *total = COSTS_N_INSNS (1);

-- 
Rask Ingemann Lambertsen

Re: RFA: Tweak optabs.c's use of constant rtx_costs

[Richard Sandiford]

Ping

Richard Sandiford <richard@codesourcery.com> writes:
> expand_binop has code like the following:
>
>   /* If we are inside an appropriately-short loop and we are optimizing,
>      force expensive constants into a register.  */
>   if (CONSTANT_P (op0) && optimize
>       && rtx_cost (op0, binoptab->code) > COSTS_N_INSNS (1))
>     {
>       if (GET_MODE (op0) != VOIDmode)
> 	op0 = convert_modes (mode, VOIDmode, op0, unsignedp);
>       op0 = force_reg (mode, op0);
>     }
>
> Outdated comment aside, what this code is doing seems perfectly
> reasonable in itself.  However, I think it's in the wrong place.
> The code is one of the first things that expand_binop does,
> so we run it even if we end up splitting the binary operation
> into smaller word-sized pieces or using a sequence of completely
> different optabs altogether.  There are two direct problems with this:
>
>   - The target has no real way of knowing whether a CONST_INT passed
>     to TARGET_RTX_COSTS is for a word or double-word operation.
>
>   - If we force a constant into a register before reducing the operation
>     into smaller pieces (such as word-mode pieces), we will end up using
>     SUBREGs of a multiword REG for those smaller pieces, instead of using
>     individual constants.  That's harder to optimise, and effectively
>     hides the individual constants until after the lower-subreg pass.
>
> This patch therefore moves the force-to-reg checks to two places:
>
>   - expand_binop_directly, before using an optab.
>
>   - The part of expand_binop that widens operands from mode M1 to
>     mode M2, if we don't care about the high bits of the widened
>     operand or result.  In this case we force the M1-mode constant
>     into a register and then generate an M2-mode paradoxical subreg.
>     (This is what we do now too, and is important for targets like ARM,
>     where we can load more HImode constants than we can their
>     sign-extended SImode equivalents.)
>
> It also moves the commutative operand canonicalisation so that
> it continues to happen after forcing constants to registers.
>
> I have some mips_rtx_costs tweaks that make things better with
> this patch but often makes things worse without it.
>
> Bootstrapped & regression-tested on x86_64-linux-gnu.  I ran CSiBE
> for arm-elf, sh-elf and x86_64-linux-gnu, all with -Os.  There were
> no changes for x86, a slight improvement for ARM:
>
> jpeg:jquant1                                      3704     3700 :   99.89%
> teem:src/nrrd/tmfKernel                         202825   202781 :   99.98%
> teem:src/echo/test/trend                         13609    12845 :   94.39%
> ----------------------------------------------------------------
> Total                                          3633455  3632643 :   99.98%
>
> and another slight improvement for SH:
>
> linux:fs/ext3/balloc                              4108     4100 :   99.81%
> linux:arch/testplatform/kernel/signal             4008     3980 :   99.30%
> linux:arch/testplatform/kernel/traps              7988     7928 :   99.25%
> teem:src/nrrd/kernel                             27716    27704 :   99.96%
> teem:src/nrrd/endianNrrd                           688      648 :   94.19%
> teem:src/nrrd/tmfKernel                         199876   199936 :  100.03%
> teem:src/limn/test/tcamanim                       3624     3620 :   99.89%
> teem:src/air/754                                  1964     1960 :   99.80%
> teem:src/echo/test/trend                         10300     8056 :   78.21%
> unrarlib:unrarlib/unrarlib                       12124    12140 :  100.13%
> ----------------------------------------------------------------
> Total                                          3183600  3181276 :   99.93%
>
> OK to install?
>
> Richard
>
>
> gcc/
> 	* optabs.c (shift_optab_p, commutative_optab_p): New functions,
> 	split out from expand_binop.
> 	(avoid_expensive_constant): New function.
> 	(expand_binop_directly): Remove commutative_op argument and
> 	call cummutative_optab_p instead.  Do not change op0 or op1
> 	when swapping xop0 and xop1.  Apply avoid_expensive_constant
> 	to each argument after potential swapping.  Enforce the
> 	canonical order of commutative operands.
> 	(expand_binop): Use shift_optab_p and commutative_optab_p.
> 	Update the calls to expand_binop_directly.  Only force constants
> 	into registers when widening an operation.  Only swap operands
> 	once a direct expansion has been rejected.
> 	(expand_twoval_binop): Only force constants into registers when
> 	using a direct expansion.
>
> Index: gcc/optabs.c
> ===================================================================
> --- gcc/optabs.c	2007-08-17 14:05:14.000000000 +0100
> +++ gcc/optabs.c	2007-08-17 14:24:49.000000000 +0100
> @@ -1246,6 +1246,56 @@ swap_commutative_operands_with_target (r
>      return rtx_equal_p (op1, target);
>  }
>  
> +/* Return true if BINOPTAB implements a shift operation.  */
> +
> +static bool
> +shift_optab_p (optab binoptab)
> +{
> +  switch (binoptab->code)
> +    {
> +    case ASHIFT:
> +    case ASHIFTRT:
> +    case LSHIFTRT:
> +    case ROTATE:
> +    case ROTATERT:
> +      return true;
> +
> +    default:
> +      return false;
> +    }
> +}
> +
> +/* Return true if BINOPTAB implements a commutatative binary operation.  */
> +
> +static bool
> +commutative_optab_p (optab binoptab)
> +{
> +  return (GET_RTX_CLASS (binoptab->code) == RTX_COMM_ARITH
> +	  || binoptab == smul_widen_optab
> +	  || binoptab == umul_widen_optab
> +	  || binoptab == smul_highpart_optab
> +	  || binoptab == umul_highpart_optab);
> +}
> +
> +/* X is to be used in mode MODE as an operand to BINOPTAB.  If we're
> +   optimizing, and if the operand is a constant that costs more than
> +   1 instruction, force the constant into a register and return that
> +   register.  Return X otherwise.  UNSIGNEDP says whether X is unsigned.  */
> +
> +static rtx
> +avoid_expensive_constant (enum machine_mode mode, optab binoptab,
> +			  rtx x, bool unsignedp)
> +{
> +  if (optimize
> +      && CONSTANT_P (x)
> +      && rtx_cost (x, binoptab->code) > COSTS_N_INSNS (1))
> +    {
> +      if (GET_MODE (x) != VOIDmode)
> +	x = convert_modes (mode, VOIDmode, x, unsignedp);
> +      x = force_reg (mode, x);
> +    }
> +  return x;
> +}
>  
>  /* Helper function for expand_binop: handle the case where there
>     is an insn that directly implements the indicated operation.
> @@ -1254,55 +1304,72 @@ swap_commutative_operands_with_target (r
>  expand_binop_directly (enum machine_mode mode, optab binoptab,
>  		       rtx op0, rtx op1,
>  		       rtx target, int unsignedp, enum optab_methods methods,
> -		       int commutative_op, rtx last)
> +		       rtx last)
>  {
>    int icode = (int) optab_handler (binoptab, mode)->insn_code;
>    enum machine_mode mode0 = insn_data[icode].operand[1].mode;
>    enum machine_mode mode1 = insn_data[icode].operand[2].mode;
>    enum machine_mode tmp_mode;
> +  bool commutative_p;
>    rtx pat;
>    rtx xop0 = op0, xop1 = op1;
>    rtx temp;
> +  rtx swap;
>    
>    if (target)
>      temp = target;
>    else
>      temp = gen_reg_rtx (mode);
> -  
> +
>    /* If it is a commutative operator and the modes would match
>       if we would swap the operands, we can save the conversions.  */
> -  if (commutative_op)
> -    {
> -      if (GET_MODE (op0) != mode0 && GET_MODE (op1) != mode1
> -	  && GET_MODE (op0) == mode1 && GET_MODE (op1) == mode0)
> -	{
> -	  rtx tmp;
> -	  
> -	  tmp = op0; op0 = op1; op1 = tmp;
> -	  tmp = xop0; xop0 = xop1; xop1 = tmp;
> -	}
> +  commutative_p = commutative_optab_p (binoptab);
> +  if (commutative_p
> +      && GET_MODE (xop0) != mode0 && GET_MODE (xop1) != mode1
> +      && GET_MODE (xop0) == mode1 && GET_MODE (xop1) == mode1)
> +    {
> +      swap = xop0;
> +      xop0 = xop1;
> +      xop1 = swap;
>      }
>    
> +  /* If we are optimizing, force expensive constants into a register.  */
> +  xop0 = avoid_expensive_constant (mode0, binoptab, xop0, unsignedp);
> +  if (!shift_optab_p (binoptab))
> +    xop1 = avoid_expensive_constant (mode1, binoptab, xop1, unsignedp);
> +
>    /* In case the insn wants input operands in modes different from
>       those of the actual operands, convert the operands.  It would
>       seem that we don't need to convert CONST_INTs, but we do, so
>       that they're properly zero-extended, sign-extended or truncated
>       for their mode.  */
>    
> -  if (GET_MODE (op0) != mode0 && mode0 != VOIDmode)
> +  if (GET_MODE (xop0) != mode0 && mode0 != VOIDmode)
>      xop0 = convert_modes (mode0,
> -			  GET_MODE (op0) != VOIDmode
> -			  ? GET_MODE (op0)
> +			  GET_MODE (xop0) != VOIDmode
> +			  ? GET_MODE (xop0)
>  			  : mode,
>  			  xop0, unsignedp);
>    
> -  if (GET_MODE (op1) != mode1 && mode1 != VOIDmode)
> +  if (GET_MODE (xop1) != mode1 && mode1 != VOIDmode)
>      xop1 = convert_modes (mode1,
> -			  GET_MODE (op1) != VOIDmode
> -			  ? GET_MODE (op1)
> +			  GET_MODE (xop1) != VOIDmode
> +			  ? GET_MODE (xop1)
>  			  : mode,
>  			  xop1, unsignedp);
>    
> +  /* If operation is commutative,
> +     try to make the first operand a register.
> +     Even better, try to make it the same as the target.
> +     Also try to make the last operand a constant.  */
> +  if (commutative_p
> +      && swap_commutative_operands_with_target (target, xop0, xop1))
> +    {
> +      swap = xop1;
> +      xop1 = xop0;
> +      xop0 = swap;
> +    }
> +
>    /* Now, if insn's predicates don't allow our operands, put them into
>       pseudo regs.  */
>    
> @@ -1375,12 +1442,6 @@ expand_binop (enum machine_mode mode, op
>    enum mode_class class;
>    enum machine_mode wider_mode;
>    rtx temp;
> -  int commutative_op = 0;
> -  int shift_op = (binoptab->code == ASHIFT
> -		  || binoptab->code == ASHIFTRT
> -		  || binoptab->code == LSHIFTRT
> -		  || binoptab->code == ROTATE
> -		  || binoptab->code == ROTATERT);
>    rtx entry_last = get_last_insn ();
>    rtx last;
>  
> @@ -1395,54 +1456,16 @@ expand_binop (enum machine_mode mode, op
>        binoptab = add_optab;
>      }
>  
> -  /* If we are inside an appropriately-short loop and we are optimizing,
> -     force expensive constants into a register.  */
> -  if (CONSTANT_P (op0) && optimize
> -      && rtx_cost (op0, binoptab->code) > COSTS_N_INSNS (1))
> -    {
> -      if (GET_MODE (op0) != VOIDmode)
> -	op0 = convert_modes (mode, VOIDmode, op0, unsignedp);
> -      op0 = force_reg (mode, op0);
> -    }
> -
> -  if (CONSTANT_P (op1) && optimize
> -      && ! shift_op && rtx_cost (op1, binoptab->code) > COSTS_N_INSNS (1))
> -    {
> -      if (GET_MODE (op1) != VOIDmode)
> -	op1 = convert_modes (mode, VOIDmode, op1, unsignedp);
> -      op1 = force_reg (mode, op1);
> -    }
> -
>    /* Record where to delete back to if we backtrack.  */
>    last = get_last_insn ();
>  
> -  /* If operation is commutative,
> -     try to make the first operand a register.
> -     Even better, try to make it the same as the target.
> -     Also try to make the last operand a constant.  */
> -  if (GET_RTX_CLASS (binoptab->code) == RTX_COMM_ARITH
> -      || binoptab == smul_widen_optab
> -      || binoptab == umul_widen_optab
> -      || binoptab == smul_highpart_optab
> -      || binoptab == umul_highpart_optab)
> -    {
> -      commutative_op = 1;
> -
> -      if (swap_commutative_operands_with_target (target, op0, op1))
> -	{
> -	  temp = op1;
> -	  op1 = op0;
> -	  op0 = temp;
> -	}
> -    }
> -
>    /* If we can do it with a three-operand insn, do so.  */
>  
>    if (methods != OPTAB_MUST_WIDEN
>        && optab_handler (binoptab, mode)->insn_code != CODE_FOR_nothing)
>      {
>        temp = expand_binop_directly (mode, binoptab, op0, op1, target,
> -				    unsignedp, methods, commutative_op, last);
> +				    unsignedp, methods, last);
>        if (temp)
>  	return temp;
>      }
> @@ -1469,8 +1492,7 @@ expand_binop (enum machine_mode mode, op
>  			       NULL_RTX, unsignedp, OPTAB_DIRECT);
>  				   
>        temp = expand_binop_directly (mode, otheroptab, op0, newop1,
> -				    target, unsignedp, methods,
> -				    commutative_op, last);
> +				    target, unsignedp, methods, last);
>        if (temp)
>  	return temp;
>      }
> @@ -1529,7 +1551,14 @@ expand_binop (enum machine_mode mode, op
>  		 || binoptab == add_optab || binoptab == sub_optab
>  		 || binoptab == smul_optab || binoptab == ashl_optab)
>  		&& class == MODE_INT)
> -	      no_extend = 1;
> +	      {
> +		no_extend = 1;
> +		xop0 = avoid_expensive_constant (mode, binoptab,
> +						 xop0, unsignedp);
> +		if (binoptab != ashl_optab)
> +		  xop1 = avoid_expensive_constant (mode, binoptab,
> +						   xop1, unsignedp);
> +	      }
>  
>  	    xop0 = widen_operand (xop0, wider_mode, mode, unsignedp, no_extend);
>  
> @@ -1558,6 +1587,18 @@ expand_binop (enum machine_mode mode, op
>  	  }
>        }
>  
> +  /* If operation is commutative,
> +     try to make the first operand a register.
> +     Even better, try to make it the same as the target.
> +     Also try to make the last operand a constant.  */
> +  if (commutative_optab_p (binoptab)
> +      && swap_commutative_operands_with_target (target, op0, op1))
> +    {
> +      temp = op1;
> +      op1 = op0;
> +      op0 = temp;
> +    }
> +
>    /* These can be done a word at a time.  */
>    if ((binoptab == and_optab || binoptab == ior_optab || binoptab == xor_optab)
>        && class == MODE_INT
> @@ -1980,7 +2021,7 @@ expand_binop (enum machine_mode mode, op
>  
>        start_sequence ();
>  
> -      if (shift_op)
> +      if (shift_optab_p (binoptab))
>  	{
>  	  op1_mode = targetm.libgcc_shift_count_mode ();
>  	  /* Specify unsigned here,
> @@ -2256,16 +2297,6 @@ expand_twoval_binop (optab binoptab, rtx
>  
>    class = GET_MODE_CLASS (mode);
>  
> -  /* If we are inside an appropriately-short loop and we are optimizing,
> -     force expensive constants into a register.  */
> -  if (CONSTANT_P (op0) && optimize
> -      && rtx_cost (op0, binoptab->code) > COSTS_N_INSNS (1))
> -    op0 = force_reg (mode, op0);
> -
> -  if (CONSTANT_P (op1) && optimize
> -      && rtx_cost (op1, binoptab->code) > COSTS_N_INSNS (1))
> -    op1 = force_reg (mode, op1);
> -
>    if (!targ0)
>      targ0 = gen_reg_rtx (mode);
>    if (!targ1)
> @@ -2282,6 +2313,10 @@ expand_twoval_binop (optab binoptab, rtx
>        rtx pat;
>        rtx xop0 = op0, xop1 = op1;
>  
> +      /* If we are optimizing, force expensive constants into a register.  */
> +      xop0 = avoid_expensive_constant (mode0, binoptab, xop0, unsignedp);
> +      xop1 = avoid_expensive_constant (mode1, binoptab, xop1, unsignedp);
> +
>        /* In case the insn wants input operands in modes different from
>  	 those of the actual operands, convert the operands.  It would
>  	 seem that we don't need to convert CONST_INTs, but we do, so

Re: RFA: Tweak optabs.c's use of constant rtx_costs

[Richard Sandiford]

Ping^2

Richard Sandiford <richard@codesourcery.com> writes:
> expand_binop has code like the following:
>
>   /* If we are inside an appropriately-short loop and we are optimizing,
>      force expensive constants into a register.  */
>   if (CONSTANT_P (op0) && optimize
>       && rtx_cost (op0, binoptab->code) > COSTS_N_INSNS (1))
>     {
>       if (GET_MODE (op0) != VOIDmode)
> 	op0 = convert_modes (mode, VOIDmode, op0, unsignedp);
>       op0 = force_reg (mode, op0);
>     }
>
> Outdated comment aside, what this code is doing seems perfectly
> reasonable in itself.  However, I think it's in the wrong place.
> The code is one of the first things that expand_binop does,
> so we run it even if we end up splitting the binary operation
> into smaller word-sized pieces or using a sequence of completely
> different optabs altogether.  There are two direct problems with this:
>
>   - The target has no real way of knowing whether a CONST_INT passed
>     to TARGET_RTX_COSTS is for a word or double-word operation.
>
>   - If we force a constant into a register before reducing the operation
>     into smaller pieces (such as word-mode pieces), we will end up using
>     SUBREGs of a multiword REG for those smaller pieces, instead of using
>     individual constants.  That's harder to optimise, and effectively
>     hides the individual constants until after the lower-subreg pass.
>
> This patch therefore moves the force-to-reg checks to two places:
>
>   - expand_binop_directly, before using an optab.
>
>   - The part of expand_binop that widens operands from mode M1 to
>     mode M2, if we don't care about the high bits of the widened
>     operand or result.  In this case we force the M1-mode constant
>     into a register and then generate an M2-mode paradoxical subreg.
>     (This is what we do now too, and is important for targets like ARM,
>     where we can load more HImode constants than we can their
>     sign-extended SImode equivalents.)
>
> It also moves the commutative operand canonicalisation so that
> it continues to happen after forcing constants to registers.
>
> I have some mips_rtx_costs tweaks that make things better with
> this patch but often makes things worse without it.
>
> Bootstrapped & regression-tested on x86_64-linux-gnu.  I ran CSiBE
> for arm-elf, sh-elf and x86_64-linux-gnu, all with -Os.  There were
> no changes for x86, a slight improvement for ARM:
>
> jpeg:jquant1                                      3704     3700 :   99.89%
> teem:src/nrrd/tmfKernel                         202825   202781 :   99.98%
> teem:src/echo/test/trend                         13609    12845 :   94.39%
> ----------------------------------------------------------------
> Total                                          3633455  3632643 :   99.98%
>
> and another slight improvement for SH:
>
> linux:fs/ext3/balloc                              4108     4100 :   99.81%
> linux:arch/testplatform/kernel/signal             4008     3980 :   99.30%
> linux:arch/testplatform/kernel/traps              7988     7928 :   99.25%
> teem:src/nrrd/kernel                             27716    27704 :   99.96%
> teem:src/nrrd/endianNrrd                           688      648 :   94.19%
> teem:src/nrrd/tmfKernel                         199876   199936 :  100.03%
> teem:src/limn/test/tcamanim                       3624     3620 :   99.89%
> teem:src/air/754                                  1964     1960 :   99.80%
> teem:src/echo/test/trend                         10300     8056 :   78.21%
> unrarlib:unrarlib/unrarlib                       12124    12140 :  100.13%
> ----------------------------------------------------------------
> Total                                          3183600  3181276 :   99.93%
>
> OK to install?
>
> Richard
>
>
> gcc/
> 	* optabs.c (shift_optab_p, commutative_optab_p): New functions,
> 	split out from expand_binop.
> 	(avoid_expensive_constant): New function.
> 	(expand_binop_directly): Remove commutative_op argument and
> 	call cummutative_optab_p instead.  Do not change op0 or op1
> 	when swapping xop0 and xop1.  Apply avoid_expensive_constant
> 	to each argument after potential swapping.  Enforce the
> 	canonical order of commutative operands.
> 	(expand_binop): Use shift_optab_p and commutative_optab_p.
> 	Update the calls to expand_binop_directly.  Only force constants
> 	into registers when widening an operation.  Only swap operands
> 	once a direct expansion has been rejected.
> 	(expand_twoval_binop): Only force constants into registers when
> 	using a direct expansion.
>
> Index: gcc/optabs.c
> ===================================================================
> --- gcc/optabs.c	2007-08-17 14:05:14.000000000 +0100
> +++ gcc/optabs.c	2007-08-17 14:24:49.000000000 +0100
> @@ -1246,6 +1246,56 @@ swap_commutative_operands_with_target (r
>      return rtx_equal_p (op1, target);
>  }
>  
> +/* Return true if BINOPTAB implements a shift operation.  */
> +
> +static bool
> +shift_optab_p (optab binoptab)
> +{
> +  switch (binoptab->code)
> +    {
> +    case ASHIFT:
> +    case ASHIFTRT:
> +    case LSHIFTRT:
> +    case ROTATE:
> +    case ROTATERT:
> +      return true;
> +
> +    default:
> +      return false;
> +    }
> +}
> +
> +/* Return true if BINOPTAB implements a commutatative binary operation.  */
> +
> +static bool
> +commutative_optab_p (optab binoptab)
> +{
> +  return (GET_RTX_CLASS (binoptab->code) == RTX_COMM_ARITH
> +	  || binoptab == smul_widen_optab
> +	  || binoptab == umul_widen_optab
> +	  || binoptab == smul_highpart_optab
> +	  || binoptab == umul_highpart_optab);
> +}
> +
> +/* X is to be used in mode MODE as an operand to BINOPTAB.  If we're
> +   optimizing, and if the operand is a constant that costs more than
> +   1 instruction, force the constant into a register and return that
> +   register.  Return X otherwise.  UNSIGNEDP says whether X is unsigned.  */
> +
> +static rtx
> +avoid_expensive_constant (enum machine_mode mode, optab binoptab,
> +			  rtx x, bool unsignedp)
> +{
> +  if (optimize
> +      && CONSTANT_P (x)
> +      && rtx_cost (x, binoptab->code) > COSTS_N_INSNS (1))
> +    {
> +      if (GET_MODE (x) != VOIDmode)
> +	x = convert_modes (mode, VOIDmode, x, unsignedp);
> +      x = force_reg (mode, x);
> +    }
> +  return x;
> +}
>  
>  /* Helper function for expand_binop: handle the case where there
>     is an insn that directly implements the indicated operation.
> @@ -1254,55 +1304,72 @@ swap_commutative_operands_with_target (r
>  expand_binop_directly (enum machine_mode mode, optab binoptab,
>  		       rtx op0, rtx op1,
>  		       rtx target, int unsignedp, enum optab_methods methods,
> -		       int commutative_op, rtx last)
> +		       rtx last)
>  {
>    int icode = (int) optab_handler (binoptab, mode)->insn_code;
>    enum machine_mode mode0 = insn_data[icode].operand[1].mode;
>    enum machine_mode mode1 = insn_data[icode].operand[2].mode;
>    enum machine_mode tmp_mode;
> +  bool commutative_p;
>    rtx pat;
>    rtx xop0 = op0, xop1 = op1;
>    rtx temp;
> +  rtx swap;
>    
>    if (target)
>      temp = target;
>    else
>      temp = gen_reg_rtx (mode);
> -  
> +
>    /* If it is a commutative operator and the modes would match
>       if we would swap the operands, we can save the conversions.  */
> -  if (commutative_op)
> -    {
> -      if (GET_MODE (op0) != mode0 && GET_MODE (op1) != mode1
> -	  && GET_MODE (op0) == mode1 && GET_MODE (op1) == mode0)
> -	{
> -	  rtx tmp;
> -	  
> -	  tmp = op0; op0 = op1; op1 = tmp;
> -	  tmp = xop0; xop0 = xop1; xop1 = tmp;
> -	}
> +  commutative_p = commutative_optab_p (binoptab);
> +  if (commutative_p
> +      && GET_MODE (xop0) != mode0 && GET_MODE (xop1) != mode1
> +      && GET_MODE (xop0) == mode1 && GET_MODE (xop1) == mode1)
> +    {
> +      swap = xop0;
> +      xop0 = xop1;
> +      xop1 = swap;
>      }
>    
> +  /* If we are optimizing, force expensive constants into a register.  */
> +  xop0 = avoid_expensive_constant (mode0, binoptab, xop0, unsignedp);
> +  if (!shift_optab_p (binoptab))
> +    xop1 = avoid_expensive_constant (mode1, binoptab, xop1, unsignedp);
> +
>    /* In case the insn wants input operands in modes different from
>       those of the actual operands, convert the operands.  It would
>       seem that we don't need to convert CONST_INTs, but we do, so
>       that they're properly zero-extended, sign-extended or truncated
>       for their mode.  */
>    
> -  if (GET_MODE (op0) != mode0 && mode0 != VOIDmode)
> +  if (GET_MODE (xop0) != mode0 && mode0 != VOIDmode)
>      xop0 = convert_modes (mode0,
> -			  GET_MODE (op0) != VOIDmode
> -			  ? GET_MODE (op0)
> +			  GET_MODE (xop0) != VOIDmode
> +			  ? GET_MODE (xop0)
>  			  : mode,
>  			  xop0, unsignedp);
>    
> -  if (GET_MODE (op1) != mode1 && mode1 != VOIDmode)
> +  if (GET_MODE (xop1) != mode1 && mode1 != VOIDmode)
>      xop1 = convert_modes (mode1,
> -			  GET_MODE (op1) != VOIDmode
> -			  ? GET_MODE (op1)
> +			  GET_MODE (xop1) != VOIDmode
> +			  ? GET_MODE (xop1)
>  			  : mode,
>  			  xop1, unsignedp);
>    
> +  /* If operation is commutative,
> +     try to make the first operand a register.
> +     Even better, try to make it the same as the target.
> +     Also try to make the last operand a constant.  */
> +  if (commutative_p
> +      && swap_commutative_operands_with_target (target, xop0, xop1))
> +    {
> +      swap = xop1;
> +      xop1 = xop0;
> +      xop0 = swap;
> +    }
> +
>    /* Now, if insn's predicates don't allow our operands, put them into
>       pseudo regs.  */
>    
> @@ -1375,12 +1442,6 @@ expand_binop (enum machine_mode mode, op
>    enum mode_class class;
>    enum machine_mode wider_mode;
>    rtx temp;
> -  int commutative_op = 0;
> -  int shift_op = (binoptab->code == ASHIFT
> -		  || binoptab->code == ASHIFTRT
> -		  || binoptab->code == LSHIFTRT
> -		  || binoptab->code == ROTATE
> -		  || binoptab->code == ROTATERT);
>    rtx entry_last = get_last_insn ();
>    rtx last;
>  
> @@ -1395,54 +1456,16 @@ expand_binop (enum machine_mode mode, op
>        binoptab = add_optab;
>      }
>  
> -  /* If we are inside an appropriately-short loop and we are optimizing,
> -     force expensive constants into a register.  */
> -  if (CONSTANT_P (op0) && optimize
> -      && rtx_cost (op0, binoptab->code) > COSTS_N_INSNS (1))
> -    {
> -      if (GET_MODE (op0) != VOIDmode)
> -	op0 = convert_modes (mode, VOIDmode, op0, unsignedp);
> -      op0 = force_reg (mode, op0);
> -    }
> -
> -  if (CONSTANT_P (op1) && optimize
> -      && ! shift_op && rtx_cost (op1, binoptab->code) > COSTS_N_INSNS (1))
> -    {
> -      if (GET_MODE (op1) != VOIDmode)
> -	op1 = convert_modes (mode, VOIDmode, op1, unsignedp);
> -      op1 = force_reg (mode, op1);
> -    }
> -
>    /* Record where to delete back to if we backtrack.  */
>    last = get_last_insn ();
>  
> -  /* If operation is commutative,
> -     try to make the first operand a register.
> -     Even better, try to make it the same as the target.
> -     Also try to make the last operand a constant.  */
> -  if (GET_RTX_CLASS (binoptab->code) == RTX_COMM_ARITH
> -      || binoptab == smul_widen_optab
> -      || binoptab == umul_widen_optab
> -      || binoptab == smul_highpart_optab
> -      || binoptab == umul_highpart_optab)
> -    {
> -      commutative_op = 1;
> -
> -      if (swap_commutative_operands_with_target (target, op0, op1))
> -	{
> -	  temp = op1;
> -	  op1 = op0;
> -	  op0 = temp;
> -	}
> -    }
> -
>    /* If we can do it with a three-operand insn, do so.  */
>  
>    if (methods != OPTAB_MUST_WIDEN
>        && optab_handler (binoptab, mode)->insn_code != CODE_FOR_nothing)
>      {
>        temp = expand_binop_directly (mode, binoptab, op0, op1, target,
> -				    unsignedp, methods, commutative_op, last);
> +				    unsignedp, methods, last);
>        if (temp)
>  	return temp;
>      }
> @@ -1469,8 +1492,7 @@ expand_binop (enum machine_mode mode, op
>  			       NULL_RTX, unsignedp, OPTAB_DIRECT);
>  				   
>        temp = expand_binop_directly (mode, otheroptab, op0, newop1,
> -				    target, unsignedp, methods,
> -				    commutative_op, last);
> +				    target, unsignedp, methods, last);
>        if (temp)
>  	return temp;
>      }
> @@ -1529,7 +1551,14 @@ expand_binop (enum machine_mode mode, op
>  		 || binoptab == add_optab || binoptab == sub_optab
>  		 || binoptab == smul_optab || binoptab == ashl_optab)
>  		&& class == MODE_INT)
> -	      no_extend = 1;
> +	      {
> +		no_extend = 1;
> +		xop0 = avoid_expensive_constant (mode, binoptab,
> +						 xop0, unsignedp);
> +		if (binoptab != ashl_optab)
> +		  xop1 = avoid_expensive_constant (mode, binoptab,
> +						   xop1, unsignedp);
> +	      }
>  
>  	    xop0 = widen_operand (xop0, wider_mode, mode, unsignedp, no_extend);
>  
> @@ -1558,6 +1587,18 @@ expand_binop (enum machine_mode mode, op
>  	  }
>        }
>  
> +  /* If operation is commutative,
> +     try to make the first operand a register.
> +     Even better, try to make it the same as the target.
> +     Also try to make the last operand a constant.  */
> +  if (commutative_optab_p (binoptab)
> +      && swap_commutative_operands_with_target (target, op0, op1))
> +    {
> +      temp = op1;
> +      op1 = op0;
> +      op0 = temp;
> +    }
> +
>    /* These can be done a word at a time.  */
>    if ((binoptab == and_optab || binoptab == ior_optab || binoptab == xor_optab)
>        && class == MODE_INT
> @@ -1980,7 +2021,7 @@ expand_binop (enum machine_mode mode, op
>  
>        start_sequence ();
>  
> -      if (shift_op)
> +      if (shift_optab_p (binoptab))
>  	{
>  	  op1_mode = targetm.libgcc_shift_count_mode ();
>  	  /* Specify unsigned here,
> @@ -2256,16 +2297,6 @@ expand_twoval_binop (optab binoptab, rtx
>  
>    class = GET_MODE_CLASS (mode);
>  
> -  /* If we are inside an appropriately-short loop and we are optimizing,
> -     force expensive constants into a register.  */
> -  if (CONSTANT_P (op0) && optimize
> -      && rtx_cost (op0, binoptab->code) > COSTS_N_INSNS (1))
> -    op0 = force_reg (mode, op0);
> -
> -  if (CONSTANT_P (op1) && optimize
> -      && rtx_cost (op1, binoptab->code) > COSTS_N_INSNS (1))
> -    op1 = force_reg (mode, op1);
> -
>    if (!targ0)
>      targ0 = gen_reg_rtx (mode);
>    if (!targ1)
> @@ -2282,6 +2313,10 @@ expand_twoval_binop (optab binoptab, rtx
>        rtx pat;
>        rtx xop0 = op0, xop1 = op1;
>  
> +      /* If we are optimizing, force expensive constants into a register.  */
> +      xop0 = avoid_expensive_constant (mode0, binoptab, xop0, unsignedp);
> +      xop1 = avoid_expensive_constant (mode1, binoptab, xop1, unsignedp);
> +
>        /* In case the insn wants input operands in modes different from
>  	 those of the actual operands, convert the operands.  It would
>  	 seem that we don't need to convert CONST_INTs, but we do, so

Re: RFA: Tweak optabs.c's use of constant rtx_costs

[Mark Mitchell]

Richard Sandiford wrote:

>> gcc/
>> 	* optabs.c (shift_optab_p, commutative_optab_p): New functions,
>> 	split out from expand_binop.
>> 	(avoid_expensive_constant): New function.
>> 	(expand_binop_directly): Remove commutative_op argument and
>> 	call cummutative_optab_p instead.  Do not change op0 or op1
>> 	when swapping xop0 and xop1.  Apply avoid_expensive_constant
>> 	to each argument after potential swapping.  Enforce the
>> 	canonical order of commutative operands.
>> 	(expand_binop): Use shift_optab_p and commutative_optab_p.
>> 	Update the calls to expand_binop_directly.  Only force constants
>> 	into registers when widening an operation.  Only swap operands
>> 	once a direct expansion has been rejected.
>> 	(expand_twoval_binop): Only force constants into registers when
>> 	using a direct expansion.

OK.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

RFA: Fix PR 33290 (fallout from Tweak optabs.c's use of constant rtx_costs)

[Richard Sandiford]

This patch caused PR 33290, an ICE for PPC on execute/930921-1.c.
The problem was with the new modes used when forcing constants
into registers:

Richard Sandiford <richard@codesourcery.com> writes:
> +/* X is to be used in mode MODE as an operand to BINOPTAB.  If we're
> +   optimizing, and if the operand is a constant that costs more than
> +   1 instruction, force the constant into a register and return that
> +   register.  Return X otherwise.  UNSIGNEDP says whether X is unsigned.  */
> +
> +static rtx
> +avoid_expensive_constant (enum machine_mode mode, optab binoptab,
> +			  rtx x, bool unsignedp)
> +{
> +  if (optimize
> +      && CONSTANT_P (x)
> +      && rtx_cost (x, binoptab->code) > COSTS_N_INSNS (1))
> +    {
> +      if (GET_MODE (x) != VOIDmode)
> +	x = convert_modes (mode, VOIDmode, x, unsignedp);
> +      x = force_reg (mode, x);
> +    }
> +  return x;
> +}
...
> +  /* If we are optimizing, force expensive constants into a register.  */
> +  xop0 = avoid_expensive_constant (mode0, binoptab, xop0, unsignedp);
> +  if (!shift_optab_p (binoptab))
> +    xop1 = avoid_expensive_constant (mode1, binoptab, xop1, unsignedp);

If an operand is a CONST_INT, it is interpreted in the mode of the
optab result.  That's what happened with the original code, but this
new code (intentionally) uses the mode required by the optab instead.
Thus the mode we pass to avoid_expansive_constant may be different
from the mode of the result, and we need to convert the integer
accordingly.

Patch bootstrapped & regression-tested on x86_64-linux-gnu,
where it introduced no new failures.  Also tested by Andreas
Tobler on PPC Darwin, and in testing by Dominique d'Humieres
(thanks to both).  OK to install?

Richard


gcc/
	PR middle-end/33290
	* optabs.c (avoid_expensive_constant): Canonicalize CONST_INTs
	before forcing them into a register.

Index: gcc/optabs.c
===================================================================
--- gcc/optabs.c	(revision 128038)
+++ gcc/optabs.c	(working copy)
@@ -1290,7 +1290,13 @@ avoid_expensive_constant (enum machine_m
       && CONSTANT_P (x)
       && rtx_cost (x, binoptab->code) > COSTS_N_INSNS (1))
     {
-      if (GET_MODE (x) != VOIDmode)
+      if (GET_CODE (x) == CONST_INT)
+	{
+	  HOST_WIDE_INT intval = trunc_int_for_mode (INTVAL (x), mode);
+	  if (intval != INTVAL (x))
+	    x = GEN_INT (intval);
+	}
+      else
 	x = convert_modes (mode, VOIDmode, x, unsignedp);
       x = force_reg (mode, x);
     }

Re: RFA: Fix PR 33290 (fallout from Tweak optabs.c's use of constant rtx_costs)

[Richard Guenther]

On 9/3/07, Richard Sandiford <richard@codesourcery.com> wrote:
> This patch caused PR 33290, an ICE for PPC on execute/930921-1.c.
> The problem was with the new modes used when forcing constants
> into registers:
>
> Richard Sandiford <richard@codesourcery.com> writes:
> > +/* X is to be used in mode MODE as an operand to BINOPTAB.  If we're
> > +   optimizing, and if the operand is a constant that costs more than
> > +   1 instruction, force the constant into a register and return that
> > +   register.  Return X otherwise.  UNSIGNEDP says whether X is unsigned.  */
> > +
> > +static rtx
> > +avoid_expensive_constant (enum machine_mode mode, optab binoptab,
> > +                       rtx x, bool unsignedp)
> > +{
> > +  if (optimize
> > +      && CONSTANT_P (x)
> > +      && rtx_cost (x, binoptab->code) > COSTS_N_INSNS (1))
> > +    {
> > +      if (GET_MODE (x) != VOIDmode)
> > +     x = convert_modes (mode, VOIDmode, x, unsignedp);
> > +      x = force_reg (mode, x);
> > +    }
> > +  return x;
> > +}
> ...
> > +  /* If we are optimizing, force expensive constants into a register.  */
> > +  xop0 = avoid_expensive_constant (mode0, binoptab, xop0, unsignedp);
> > +  if (!shift_optab_p (binoptab))
> > +    xop1 = avoid_expensive_constant (mode1, binoptab, xop1, unsignedp);
>
> If an operand is a CONST_INT, it is interpreted in the mode of the
> optab result.  That's what happened with the original code, but this
> new code (intentionally) uses the mode required by the optab instead.
> Thus the mode we pass to avoid_expansive_constant may be different
> from the mode of the result, and we need to convert the integer
> accordingly.
>
> Patch bootstrapped & regression-tested on x86_64-linux-gnu,
> where it introduced no new failures.  Also tested by Andreas
> Tobler on PPC Darwin, and in testing by Dominique d'Humieres
> (thanks to both).  OK to install?
>

Ok.

Thanks,
Richard.

>
>
> gcc/
>         PR middle-end/33290
>         * optabs.c (avoid_expensive_constant): Canonicalize CONST_INTs
>         before forcing them into a register.

RFA: Fix PR 33306 (fallout from Tweak optabs.c's use of constant rtx_costs)

[Richard Sandiford]

Sorry, PR 33306 is second fallout from my optabs.c patch, this time on
Alpha.  A (const_double:TF 1.0) is an expensive constant on Alpha,
and Alpha's divtf3 expander does not specify a mode for the operands.
So avoid_expensive_constant ends up trying to force the constant into a
VOIDmode register:

Richard Sandiford <richard@codesourcery.com> writes:
> +/* X is to be used in mode MODE as an operand to BINOPTAB.  If we're
> +   optimizing, and if the operand is a constant that costs more than
> +   1 instruction, force the constant into a register and return that
> +   register.  Return X otherwise.  UNSIGNEDP says whether X is unsigned.  */
> +
> +static rtx
> +avoid_expensive_constant (enum machine_mode mode, optab binoptab,
> +			  rtx x, bool unsignedp)
> +{
> +  if (optimize
> +      && CONSTANT_P (x)
> +      && rtx_cost (x, binoptab->code) > COSTS_N_INSNS (1))
> +    {
> +      if (GET_MODE (x) != VOIDmode)
> +	x = convert_modes (mode, VOIDmode, x, unsignedp);
> +      x = force_reg (mode, x);
> +    }
> +  return x;
> +}
...
> +  /* If we are optimizing, force expensive constants into a register.  */
> +  xop0 = avoid_expensive_constant (mode0, binoptab, xop0, unsignedp);
> +  if (!shift_optab_p (binoptab))
> +    xop1 = avoid_expensive_constant (mode1, binoptab, xop1, unsignedp);

When mode0 or mode1 is VOIDmode, we could fall back to using the
result mode instead.  Or avoid_expensive_constant could use the
mode of X when the mode is VOIDmode, although this doesn't fix
the case where X is a CONST_INT and the mode is VOIDmode.
However, expand_binop_directly is careful to pass unmodified
operands when the expander specifies no mode, so I think that's
the right fix here too.

Bootstrapped & regression-tested on x86_64-linux-gnu.  Serge Belyshev
reports that it fixes C-only bootstrap on Alpha too.  OK to install?

Richard


gcc/
	PR middle-end/33306
	* optabs.c (avoid_expensive_constant): Do nothing if MODE is VOIDmode.

Index: gcc/optabs.c
===================================================================
--- gcc/optabs.c	(revision 128145)
+++ gcc/optabs.c	(working copy)
@@ -1359,7 +1359,8 @@ commutative_optab_p (optab binoptab)
 avoid_expensive_constant (enum machine_mode mode, optab binoptab,
 			  rtx x, bool unsignedp)
 {
-  if (optimize
+  if (mode != VOIDmode
+      && optimize
       && CONSTANT_P (x)
       && rtx_cost (x, binoptab->code) > COSTS_N_INSNS (1))
     {

Re: RFA: Fix PR 33306 (fallout from Tweak optabs.c's use of constant rtx_costs)

[Richard Guenther]

On 9/6/07, Richard Sandiford <richard@codesourcery.com> wrote:
> Sorry, PR 33306 is second fallout from my optabs.c patch, this time on
> Alpha.  A (const_double:TF 1.0) is an expensive constant on Alpha,
> and Alpha's divtf3 expander does not specify a mode for the operands.
> So avoid_expensive_constant ends up trying to force the constant into a
> VOIDmode register:
>
> Richard Sandiford <richard@codesourcery.com> writes:
> > +/* X is to be used in mode MODE as an operand to BINOPTAB.  If we're
> > +   optimizing, and if the operand is a constant that costs more than
> > +   1 instruction, force the constant into a register and return that
> > +   register.  Return X otherwise.  UNSIGNEDP says whether X is unsigned.  */
> > +
> > +static rtx
> > +avoid_expensive_constant (enum machine_mode mode, optab binoptab,
> > +                       rtx x, bool unsignedp)
> > +{
> > +  if (optimize
> > +      && CONSTANT_P (x)
> > +      && rtx_cost (x, binoptab->code) > COSTS_N_INSNS (1))
> > +    {
> > +      if (GET_MODE (x) != VOIDmode)
> > +     x = convert_modes (mode, VOIDmode, x, unsignedp);
> > +      x = force_reg (mode, x);
> > +    }
> > +  return x;
> > +}
> ...
> > +  /* If we are optimizing, force expensive constants into a register.  */
> > +  xop0 = avoid_expensive_constant (mode0, binoptab, xop0, unsignedp);
> > +  if (!shift_optab_p (binoptab))
> > +    xop1 = avoid_expensive_constant (mode1, binoptab, xop1, unsignedp);
>
> When mode0 or mode1 is VOIDmode, we could fall back to using the
> result mode instead.  Or avoid_expensive_constant could use the
> mode of X when the mode is VOIDmode, although this doesn't fix
> the case where X is a CONST_INT and the mode is VOIDmode.
> However, expand_binop_directly is careful to pass unmodified
> operands when the expander specifies no mode, so I think that's
> the right fix here too.
>
> Bootstrapped & regression-tested on x86_64-linux-gnu.  Serge Belyshev
> reports that it fixes C-only bootstrap on Alpha too.  OK to install?

Ok.

Thanks,
Richard.