[PATCH, ifcvt] Check size cost in noce_try_store_flag_mask

[PATCH, ifcvt] Check size cost in noce_try_store_flag_mask

[Zhenqiang Chen]

Hi,

Function noce_try_store_flag_mask converts "if (test) x = 0;" to "x &=
-(test == 0);"

But from code size view, "x &= -(test == 0);" might have more instructions
than "if (test) x = 0;". The patch checks the cost to determine the
conversion is valuable or not.

Bootstrap and no make check regression on X86-64.
No make check regression with Cortex-M0 qemu.
For CSiBE, ARM Cortex-m0 result is a little better. A little regression for
MIPS. Roughly no change for PowerPC.

OK for trunk?

Thanks!
-Zhenqiang

ChangeLog:
2014-10-27  Zhenqiang Chen  <zhenqiang.chen@arm.com>

	* ifcvt.c (noce_try_store_flag_mask): Check rtx cost.

testsuite/ChangeLog:
2014-10-27  Zhenqiang Chen  <zhenqiang.chen@arm.com>

	* gcc.target/arm/ifcvt-size-check.c: New test.

diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index 949d2b4..3abd518 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -1393,6 +1393,14 @@ noce_try_store_flag_mask (struct noce_if_info
*if_info)
 	  if (!seq)
 	    return FALSE;
 
+	  if (optimize_function_for_size_p (cfun))
+	    {
+	      int old_cost = COSTS_N_INSNS (if_info->branch_cost + 1);
+	      int new_cost = seq_cost (seq, 0);
+	      if (new_cost > old_cost)
+		return FALSE;
+	    }
+
 	  emit_insn_before_setloc (seq, if_info->jump,
 				   INSN_LOCATION (if_info->insn_a));
 	  return TRUE;
diff --git a/gcc/testsuite/gcc.target/arm/ifcvt-size-check.c
b/gcc/testsuite/gcc.target/arm/ifcvt-size-check.c
new file mode 100644
index 0000000..43fa16b
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/ifcvt-size-check.c
@@ -0,0 +1,13 @@
+/* { dg-do assemble } */
+/* { dg-options "-mthumb -Os " }  */
+/* { dg-require-effective-target arm_thumb1_ok } */
+
+int
+test (unsigned char iov_len, int count, int i)
+{
+  unsigned char bytes = 0;
+  if ((unsigned char) ((char) 127 - bytes) < iov_len)
+    return 22;
+  return 0;
+}
+/* { dg-final { object-size text <= 12 } } */

Re: [PATCH, ifcvt] Check size cost in noce_try_store_flag_mask

[Andrew Pinski]

On Sun, Oct 26, 2014 at 6:53 PM, Zhenqiang Chen <zhenqiang.chen@arm.com> wrote:
> Hi,
>
> Function noce_try_store_flag_mask converts "if (test) x = 0;" to "x &=
> -(test == 0);"
>
> But from code size view, "x &= -(test == 0);" might have more instructions
> than "if (test) x = 0;". The patch checks the cost to determine the
> conversion is valuable or not.
>
> Bootstrap and no make check regression on X86-64.
> No make check regression with Cortex-M0 qemu.
> For CSiBE, ARM Cortex-m0 result is a little better. A little regression for
> MIPS. Roughly no change for PowerPC.
>
> OK for trunk?
>
> Thanks!
> -Zhenqiang
>
> ChangeLog:
> 2014-10-27  Zhenqiang Chen  <zhenqiang.chen@arm.com>
>
>         * ifcvt.c (noce_try_store_flag_mask): Check rtx cost.
>
> testsuite/ChangeLog:
> 2014-10-27  Zhenqiang Chen  <zhenqiang.chen@arm.com>
>
>         * gcc.target/arm/ifcvt-size-check.c: New test.
>
> diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
> index 949d2b4..3abd518 100644
> --- a/gcc/ifcvt.c
> +++ b/gcc/ifcvt.c
> @@ -1393,6 +1393,14 @@ noce_try_store_flag_mask (struct noce_if_info
> *if_info)
>           if (!seq)
>             return FALSE;
>
> +         if (optimize_function_for_size_p (cfun))
> +           {
> +             int old_cost = COSTS_N_INSNS (if_info->branch_cost + 1);
> +             int new_cost = seq_cost (seq, 0);
> +             if (new_cost > old_cost)
> +               return FALSE;
> +           }


Why not do it unconditionally rather than base this on optimize for size?
If the costs are incorrect for non optimize for size, we need to fix those.

Thanks,
Andrew Pinski

> +
>           emit_insn_before_setloc (seq, if_info->jump,
>                                    INSN_LOCATION (if_info->insn_a));
>           return TRUE;
> diff --git a/gcc/testsuite/gcc.target/arm/ifcvt-size-check.c
> b/gcc/testsuite/gcc.target/arm/ifcvt-size-check.c
> new file mode 100644
> index 0000000..43fa16b
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arm/ifcvt-size-check.c
> @@ -0,0 +1,13 @@
> +/* { dg-do assemble } */
> +/* { dg-options "-mthumb -Os " }  */
> +/* { dg-require-effective-target arm_thumb1_ok } */
> +
> +int
> +test (unsigned char iov_len, int count, int i)
> +{
> +  unsigned char bytes = 0;
> +  if ((unsigned char) ((char) 127 - bytes) < iov_len)
> +    return 22;
> +  return 0;
> +}
> +/* { dg-final { object-size text <= 12 } } */
>
>
>

RE: [PATCH, ifcvt] Check size cost in noce_try_store_flag_mask

[Matthew Fortune]

Zhenqiang Chen <zhenqiang.chen@arm.com> writes:
> For CSiBE, ARM Cortex-m0 result is a little better. A little regression
> for
> MIPS. Roughly no change for PowerPC.

Do I take it that a little regression for MIPS is so small it can be
considered noise? I haven't had chance to run CSiBE to see the difference.

Thanks,
Matthew

> 
> OK for trunk?
> 
> Thanks!
> -Zhenqiang
> 
> ChangeLog:
> 2014-10-27  Zhenqiang Chen  <zhenqiang.chen@arm.com>
> 
> 	* ifcvt.c (noce_try_store_flag_mask): Check rtx cost.
> 
> testsuite/ChangeLog:
> 2014-10-27  Zhenqiang Chen  <zhenqiang.chen@arm.com>
> 
> 	* gcc.target/arm/ifcvt-size-check.c: New test.
> 
> diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
> index 949d2b4..3abd518 100644
> --- a/gcc/ifcvt.c
> +++ b/gcc/ifcvt.c
> @@ -1393,6 +1393,14 @@ noce_try_store_flag_mask (struct noce_if_info
> *if_info)
>  	  if (!seq)
>  	    return FALSE;
> 
> +	  if (optimize_function_for_size_p (cfun))
> +	    {
> +	      int old_cost = COSTS_N_INSNS (if_info->branch_cost + 1);
> +	      int new_cost = seq_cost (seq, 0);
> +	      if (new_cost > old_cost)
> +		return FALSE;
> +	    }
> +
>  	  emit_insn_before_setloc (seq, if_info->jump,
>  				   INSN_LOCATION (if_info->insn_a));
>  	  return TRUE;
> diff --git a/gcc/testsuite/gcc.target/arm/ifcvt-size-check.c
> b/gcc/testsuite/gcc.target/arm/ifcvt-size-check.c
> new file mode 100644
> index 0000000..43fa16b
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arm/ifcvt-size-check.c
> @@ -0,0 +1,13 @@
> +/* { dg-do assemble } */
> +/* { dg-options "-mthumb -Os " }  */
> +/* { dg-require-effective-target arm_thumb1_ok } */
> +
> +int
> +test (unsigned char iov_len, int count, int i)
> +{
> +  unsigned char bytes = 0;
> +  if ((unsigned char) ((char) 127 - bytes) < iov_len)
> +    return 22;
> +  return 0;
> +}
> +/* { dg-final { object-size text <= 12 } } */
> 
> 

Re: [PATCH, ifcvt] Check size cost in noce_try_store_flag_mask

[Jeff Law]

On 10/26/14 19:53, Zhenqiang Chen wrote:
> Hi,
>
> Function noce_try_store_flag_mask converts "if (test) x = 0;" to "x &=
> -(test == 0);"
>
> But from code size view, "x &= -(test == 0);" might have more instructions
> than "if (test) x = 0;". The patch checks the cost to determine the
> conversion is valuable or not.
>
> Bootstrap and no make check regression on X86-64.
> No make check regression with Cortex-M0 qemu.
> For CSiBE, ARM Cortex-m0 result is a little better. A little regression for
> MIPS. Roughly no change for PowerPC.
>
> OK for trunk?
>
> Thanks!
> -Zhenqiang
>
> ChangeLog:
> 2014-10-27  Zhenqiang Chen  <zhenqiang.chen@arm.com>
>
> 	* ifcvt.c (noce_try_store_flag_mask): Check rtx cost.
>
> testsuite/ChangeLog:
> 2014-10-27  Zhenqiang Chen  <zhenqiang.chen@arm.com>
>
> 	* gcc.target/arm/ifcvt-size-check.c: New test.
>
> diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
> index 949d2b4..3abd518 100644
> --- a/gcc/ifcvt.c
> +++ b/gcc/ifcvt.c
> @@ -1393,6 +1393,14 @@ noce_try_store_flag_mask (struct noce_if_info
> *if_info)
>   	  if (!seq)
>   	    return FALSE;
>
> +	  if (optimize_function_for_size_p (cfun))
> +	    {
> +	      int old_cost = COSTS_N_INSNS (if_info->branch_cost + 1);
> +	      int new_cost = seq_cost (seq, 0);
> +	      if (new_cost > old_cost)
> +		return FALSE;
> +	    }
> +
As Andrew pointed out, there's really no reason to limit this to cases 
where we're optimizing for size.  So that check should be removed.

You should also engage with Michael to determine if the MIPS regressions 
are significant enough to warrant deeper investigation.  My gut tells me 
that if MIPS is regressing because of this, then that's going to be an 
issue in the MIPS costing model that the MIPS folks will ultimately need 
to fix.

jeff

Re: [PATCH, ifcvt] Check size cost in noce_try_store_flag_mask

[Jeff Law]

On 10/31/14 00:30, Zhenqiang Chen wrote:
> Thank you all for the comments. Patch is updated.
>
> Bootstrap and no make check regression on X86-64.
> No make check regression with Cortex-M0 qemu.
> No performance changes for coremark, dhrystone, spec2000 and spec2006 on
> X86-64 and Cortex-A15.
>
> For CSiBE, ARM Cortex-M0 result is a little better. A little regression for
> MIPS (less than 0.01%).
Given Matthew's follow-up indicting he was comfortable with the patch 
as-is and the MIPS folks will follow-up on their costing models, this 
patch is fine.

jeff

Re: [PATCH, ifcvt] Check size cost in noce_try_store_flag_mask

[pinskia]

> On Oct 31, 2014, at 4:07 AM, Matthew Fortune <Matthew.Fortune@imgtec.com> wrote:
> 
> Andrew Pinski <pinskia@gmail.com> writes:
>> On Thu, Oct 30, 2014 at 11:30 PM, Zhenqiang Chen <zhenqiang.chen@arm.com>
>> wrote:
>>> Thank you all for the comments. Patch is updated.
>>> 
>>> Bootstrap and no make check regression on X86-64.
>>> No make check regression with Cortex-M0 qemu.
>>> No performance changes for coremark, dhrystone, spec2000 and spec2006 on
>>> X86-64 and Cortex-A15.
>>> 
>>> For CSiBE, ARM Cortex-M0 result is a little better. A little regression
>> for
>>> MIPS (less than 0.01%).
>> 
>> I think I have a fix for MIPS which I need to submit too.  The problem
>> is IF_THEN_ELSE is not implemented for mips_rtx_costs.
>> 
>> Something like the attached one (though It is not updated for the new
>> cores including octeon3).
> 
> This looks OK in principle so I have no objection to the original patch
> from Zhengiang. The MIPS patch can follow on.
> 
> Andrew: Are you setting higher costs for octeon to try and avoid the
> conversion owing to high latency for MOV[NZ] etc in octeon*?

Yes. In fact I was doing it for the higher latency on octeon 2 than Octeon 1/+. I saw a small improvement with that, using other instructions in one or two cases which be scheduled with other code. 


> Should that
> be conditional on speed vs size?

Yes though I thought we had a variable for size too. 

Thanks,
Andrew

> 
> Thanks,
> Matthew

RE: [PATCH, ifcvt] Check size cost in noce_try_store_flag_mask

[Matthew Fortune]

Andrew Pinski <pinskia@gmail.com> writes:
> On Thu, Oct 30, 2014 at 11:30 PM, Zhenqiang Chen <zhenqiang.chen@arm.com>
> wrote:
> > Thank you all for the comments. Patch is updated.
> >
> > Bootstrap and no make check regression on X86-64.
> > No make check regression with Cortex-M0 qemu.
> > No performance changes for coremark, dhrystone, spec2000 and spec2006 on
> > X86-64 and Cortex-A15.
> >
> > For CSiBE, ARM Cortex-M0 result is a little better. A little regression
> for
> > MIPS (less than 0.01%).
> 
> I think I have a fix for MIPS which I need to submit too.  The problem
> is IF_THEN_ELSE is not implemented for mips_rtx_costs.
> 
> Something like the attached one (though It is not updated for the new
> cores including octeon3).

This looks OK in principle so I have no objection to the original patch
from Zhengiang. The MIPS patch can follow on.

Andrew: Are you setting higher costs for octeon to try and avoid the
conversion owing to high latency for MOV[NZ] etc in octeon*? Should that
be conditional on speed vs size?

Thanks,
Matthew

Re: [PATCH, ifcvt] Check size cost in noce_try_store_flag_mask

[Andrew Pinski]

[-- Attachment #1: Type: text/plain, Size: 4564 bytes --]

On Thu, Oct 30, 2014 at 11:30 PM, Zhenqiang Chen <zhenqiang.chen@arm.com> wrote:
> Thank you all for the comments. Patch is updated.
>
> Bootstrap and no make check regression on X86-64.
> No make check regression with Cortex-M0 qemu.
> No performance changes for coremark, dhrystone, spec2000 and spec2006 on
> X86-64 and Cortex-A15.
>
> For CSiBE, ARM Cortex-M0 result is a little better. A little regression for
> MIPS (less than 0.01%).

I think I have a fix for MIPS which I need to submit too.  The problem
is IF_THEN_ELSE is not implemented for mips_rtx_costs.

Something like the attached one (though It is not updated for the new
cores including octeon3).

Thanks,
Andrew Pinski

>
> diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
> index 653653f..7bb2578 100644
> --- a/gcc/ifcvt.c
> +++ b/gcc/ifcvt.c
> @@ -1393,6 +1393,9 @@ noce_try_store_flag_mask (struct noce_if_info
> *if_info)
>
>        if (target)
>         {
> +         int old_cost, new_cost, insn_cost;
> +         int speed_p;
> +
>           if (target != if_info->x)
>             noce_emit_move_insn (if_info->x, target);
>
> @@ -1400,6 +1403,14 @@ noce_try_store_flag_mask (struct noce_if_info
> *if_info)
>           if (!seq)
>             return FALSE;
>
> +         speed_p = optimize_bb_for_speed_p (BLOCK_FOR_INSN
> (if_info->insn_a));
> +         insn_cost = insn_rtx_cost (PATTERN (if_info->insn_a), speed_p);
> +         old_cost = COSTS_N_INSNS (if_info->branch_cost) + insn_cost;
> +         new_cost = seq_cost (seq, speed_p);
> +
> +         if (new_cost > old_cost)
> +           return FALSE;
> +
>           emit_insn_before_setloc (seq, if_info->jump,
>                                    INSN_LOCATION (if_info->insn_a));
>           return TRUE;
> diff --git a/gcc/testsuite/gcc.target/arm/ifcvt-size-check.c
> b/gcc/testsuite/gcc.target/arm/ifcvt-size-check.c
> new file mode 100644
> index 0000000..43fa16b
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arm/ifcvt-size-check.c
> @@ -0,0 +1,13 @@
> +/* { dg-do assemble } */
> +/* { dg-options "-mthumb -Os " }  */
> +/* { dg-require-effective-target arm_thumb1_ok } */
> +
> +int
> +test (unsigned char iov_len, int count, int i) {
> +  unsigned char bytes = 0;
> +  if ((unsigned char) ((char) 127 - bytes) < iov_len)
> +    return 22;
> +  return 0;
> +}
> +/* { dg-final { object-size text <= 12 } } */
>
>
>> -----Original Message-----
>> From: Jeff Law [mailto:law@redhat.com]
>> Sent: Thursday, October 30, 2014 1:27 PM
>> To: Zhenqiang Chen; gcc-patches@gcc.gnu.org
>> Subject: Re: [PATCH, ifcvt] Check size cost in noce_try_store_flag_mask
>>
>> On 10/26/14 19:53, Zhenqiang Chen wrote:
>> > Hi,
>> >
>> > Function noce_try_store_flag_mask converts "if (test) x = 0;" to "x &=
>> > -(test == 0);"
>> >
>> > But from code size view, "x &= -(test == 0);" might have more
>> > instructions than "if (test) x = 0;". The patch checks the cost to
>> > determine the conversion is valuable or not.
>> >
>> > Bootstrap and no make check regression on X86-64.
>> > No make check regression with Cortex-M0 qemu.
>> > For CSiBE, ARM Cortex-m0 result is a little better. A little
>> > regression for MIPS. Roughly no change for PowerPC.
>> >
>> > OK for trunk?
>> >
>> > Thanks!
>> > -Zhenqiang
>> >
>> > ChangeLog:
>> > 2014-10-27  Zhenqiang Chen  <zhenqiang.chen@arm.com>
>> >
>> >     * ifcvt.c (noce_try_store_flag_mask): Check rtx cost.
>> >
>> > testsuite/ChangeLog:
>> > 2014-10-27  Zhenqiang Chen  <zhenqiang.chen@arm.com>
>> >
>> >     * gcc.target/arm/ifcvt-size-check.c: New test.
>> >
>> > diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c index 949d2b4..3abd518 100644
>> > --- a/gcc/ifcvt.c
>> > +++ b/gcc/ifcvt.c
>> > @@ -1393,6 +1393,14 @@ noce_try_store_flag_mask (struct noce_if_info
>> > *if_info)
>> >       if (!seq)
>> >         return FALSE;
>> >
>> > +     if (optimize_function_for_size_p (cfun))
>> > +       {
>> > +         int old_cost = COSTS_N_INSNS (if_info->branch_cost + 1);
>> > +         int new_cost = seq_cost (seq, 0);
>> > +         if (new_cost > old_cost)
>> > +           return FALSE;
>> > +       }
>> > +
>> As Andrew pointed out, there's really no reason to limit this to cases
> where
>> we're optimizing for size.  So that check should be removed.
>>
>> You should also engage with Michael to determine if the MIPS regressions
>> are significant enough to warrant deeper investigation.  My gut tells me
> that
>> if MIPS is regressing because of this, then that's going to be an issue in
> the
>> MIPS costing model that the MIPS folks will ultimately need to fix.
>>
>> jeff
>>
>
>
>
>

[-- Attachment #2: gcc.git-f37a94987366f2464c7122fa66b8f50797a26f11.patch --]
[-- Type: application/octet-stream, Size: 12561 bytes --]

From: Andrew Pinski <apinski@cavium.com>
Date: Tue, 17 Jul 2012 02:21:23 +0000 (-0700)
Subject: 2012-05-24  Andrew Pinski  <apinski@cavium.com>
X-Git-Tag: tc47_SDK_3_0_0_3~121
X-Git-Url: http://cagit1.caveonetworks.com/git/?p=toolchain%2Fgcc.git;a=commitdiff_plain;h=f37a94987366f2464c7122fa66b8f50797a26f11

2012-05-24  Andrew Pinski  <apinski@cavium.com>

        start of bug #4296
        * config/mips/mips.c (mips_rtx_cost_data): Add movz cost.
	(mips_cpu_info_table): Set movz for Octeon to 2 and Octeon2 to 3.
	(mips_rtx_costs <case IF_THEN_ELSE>): Return mips_cost->movz.
---

diff --git a/gcc/ChangeLog.CAVIUM b/gcc/ChangeLog.CAVIUM
index 38eb900..454eb71 100644
--- a/gcc/ChangeLog.CAVIUM
+++ b/gcc/ChangeLog.CAVIUM
@@ -1,10 +1,16 @@
+2012-05-24  Andrew Pinski  <apinski@cavium.com>
+
+        start of bug #4296
+        * config/mips/mips.c (mips_rtx_cost_data): Add movz cost.
+	(mips_cpu_info_table): Set movz for Octeon to 2 and Octeon2 to 3.
+	(mips_rtx_costs <case IF_THEN_ELSE>): Return mips_cost->movz.
+
 2012-07-15  Andrew Pinski  <apinski@caviumnetworks.com>
 
         Bug #3154
         * config/mips/mips.md (bswapdi2): New instruction pattern.
         (bswapsi2): Likewise.
         * config/mips/mips.h (ISA_HAS_SBWH): New ISA define.
-        * testsuite/gcc.c-torture/execute/bswap-1.c: New test.
         * testsuite/gcc.target/mips/bswap-1.c: New test.
 
 2012-07-15  Andrew Pinski  <apinski@cavium.com>
diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index 775326c..2b9baf5 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -476,6 +476,7 @@ struct mips_rtx_cost_data
   unsigned short int_mult_di;
   unsigned short int_div_si;
   unsigned short int_div_di;
+  unsigned short movz;
   unsigned short branch_cost;
   unsigned short memory_latency;
 };
@@ -693,6 +694,7 @@ static const struct mips_cpu_info mips_cpu_info_table[] = {
                       COSTS_N_INSNS (10), /* int_mult_di */  \
                       COSTS_N_INSNS (69), /* int_div_si */   \
                       COSTS_N_INSNS (69), /* int_div_di */   \
+		      COSTS_N_INSNS (1),  /* movz */         \
                                        2, /* branch_cost */  \
                                        4  /* memory_latency */
 
@@ -715,6 +717,7 @@ static const struct mips_rtx_cost_data mips_rtx_cost_optimize_size = {
   COSTS_N_INSNS (1),            /* int_mult_di */
   COSTS_N_INSNS (1),            /* int_div_si */
   COSTS_N_INSNS (1),            /* int_div_di */
+  COSTS_N_INSNS (1),		/* movz */
 		   2,           /* branch_cost */
 		   4            /* memory_latency */
 };
@@ -732,6 +735,7 @@ static const struct mips_rtx_cost_data
     COSTS_N_INSNS (12),           /* int_mult_di */
     COSTS_N_INSNS (35),           /* int_div_si */
     COSTS_N_INSNS (35),           /* int_div_di */
+    COSTS_N_INSNS (1),		  /* movz */
 		     1,           /* branch_cost */
 		     4            /* memory_latency */
   },
@@ -741,6 +745,7 @@ static const struct mips_rtx_cost_data
     COSTS_N_INSNS (6),            /* int_mult_di */
     COSTS_N_INSNS (36),           /* int_div_si */
     COSTS_N_INSNS (36),           /* int_div_di */
+    COSTS_N_INSNS (1),		  /* movz */
 		     1,           /* branch_cost */
 		     4            /* memory_latency */
   },
@@ -750,6 +755,7 @@ static const struct mips_rtx_cost_data
     COSTS_N_INSNS (36),           /* int_mult_di */
     COSTS_N_INSNS (37),           /* int_div_si */
     COSTS_N_INSNS (37),           /* int_div_di */
+    COSTS_N_INSNS (1),		  /* movz */
 		     1,           /* branch_cost */
 		     4            /* memory_latency */
   },
@@ -759,6 +765,7 @@ static const struct mips_rtx_cost_data
     COSTS_N_INSNS (11),           /* int_mult_di */
     COSTS_N_INSNS (36),           /* int_div_si */
     COSTS_N_INSNS (68),           /* int_div_di */
+    COSTS_N_INSNS (1),		  /* movz */
 		     1,           /* branch_cost */
 		     4            /* memory_latency */
   },
@@ -772,6 +779,7 @@ static const struct mips_rtx_cost_data
     COSTS_N_INSNS (11),           /* int_mult_di */
     COSTS_N_INSNS (36),           /* int_div_si */
     COSTS_N_INSNS (68),           /* int_div_di */
+    COSTS_N_INSNS (1),		  /* movz */
 		     1,           /* branch_cost */
 		     4            /* memory_latency */
   },
@@ -785,6 +793,7 @@ static const struct mips_rtx_cost_data
     COSTS_N_INSNS (7),            /* int_mult_di */
     COSTS_N_INSNS (42),           /* int_div_si */
     COSTS_N_INSNS (72),           /* int_div_di */
+    COSTS_N_INSNS (1),		  /* movz */
 		     1,           /* branch_cost */
 		     4            /* memory_latency */
   },
@@ -794,6 +803,7 @@ static const struct mips_rtx_cost_data
     COSTS_N_INSNS (5),            /* int_mult_di */
     COSTS_N_INSNS (41),           /* int_div_si */
     COSTS_N_INSNS (41),           /* int_div_di */
+    COSTS_N_INSNS (1),		  /* movz */
 		     1,           /* branch_cost */
 		     4            /* memory_latency */
   },
@@ -807,6 +817,7 @@ static const struct mips_rtx_cost_data
     COSTS_N_INSNS (5),            /* int_mult_di */
     COSTS_N_INSNS (41),           /* int_div_si */
     COSTS_N_INSNS (41),           /* int_div_di */
+    COSTS_N_INSNS (1),		  /* movz */
 		     1,           /* branch_cost */
 		     4            /* memory_latency */
   },
@@ -820,6 +831,7 @@ static const struct mips_rtx_cost_data
     COSTS_N_INSNS (5),            /* int_mult_di */
     COSTS_N_INSNS (41),           /* int_div_si */
     COSTS_N_INSNS (41),           /* int_div_di */
+    COSTS_N_INSNS (1),		  /* movz */
 		     1,           /* branch_cost */
 		     4            /* memory_latency */
   },
@@ -829,6 +841,7 @@ static const struct mips_rtx_cost_data
     COSTS_N_INSNS (5),            /* int_mult_di */
     COSTS_N_INSNS (41),           /* int_div_si */
     COSTS_N_INSNS (41),           /* int_div_di */
+    COSTS_N_INSNS (1),		  /* movz */
 		     1,           /* branch_cost */
 		     4            /* memory_latency */
   },
@@ -842,6 +855,7 @@ static const struct mips_rtx_cost_data
     COSTS_N_INSNS (5),            /* int_mult_di */
     COSTS_N_INSNS (41),           /* int_div_si */
     COSTS_N_INSNS (41),           /* int_div_di */
+    COSTS_N_INSNS (1),		  /* movz */
 		     1,           /* branch_cost */
 		     4            /* memory_latency */
   },
@@ -855,6 +869,7 @@ static const struct mips_rtx_cost_data
     COSTS_N_INSNS (5),            /* int_mult_di */
     COSTS_N_INSNS (41),           /* int_div_si */
     COSTS_N_INSNS (41),           /* int_div_di */
+    COSTS_N_INSNS (1),		  /* movz */
 		     1,           /* branch_cost */
 		     4            /* memory_latency */
   },
@@ -868,6 +883,7 @@ static const struct mips_rtx_cost_data
     COSTS_N_INSNS (5),            /* int_mult_di */
     COSTS_N_INSNS (41),           /* int_div_si */
     COSTS_N_INSNS (41),           /* int_div_di */
+    COSTS_N_INSNS (1),		  /* movz */
 		     1,           /* branch_cost */
 		     4            /* memory_latency */
   },
@@ -890,7 +906,8 @@ static const struct mips_rtx_cost_data
     COSTS_N_INSNS (5),            /* int_mult_di */
     COSTS_N_INSNS (72),           /* int_div_si */
     COSTS_N_INSNS (72),           /* int_div_di */
-                     1,		  /* branch_cost */
+    COSTS_N_INSNS (2),		  /* movz */
+                     4,		  /* branch_cost */
                      4		  /* memory_latency */
   },
     /* Octeon II */
@@ -900,6 +917,7 @@ static const struct mips_rtx_cost_data
     COSTS_N_INSNS (6),            /* int_mult_di */
     COSTS_N_INSNS (18),           /* int_div_si */
     COSTS_N_INSNS (35),           /* int_div_di */
+    COSTS_N_INSNS (3),		  /* movz */
                      4,		  /* branch_cost */
                      4		  /* memory_latency */
   },
@@ -913,6 +931,7 @@ static const struct mips_rtx_cost_data
     COSTS_N_INSNS (2),            /* int_mult_di */
     COSTS_N_INSNS (35),           /* int_div_si */
     COSTS_N_INSNS (35),           /* int_div_di */
+    COSTS_N_INSNS (1),		  /* movz */
 		     1,           /* branch_cost */
 		     4            /* memory_latency */
   },
@@ -926,6 +945,7 @@ static const struct mips_rtx_cost_data
     COSTS_N_INSNS (17),           /* int_mult_di */
     COSTS_N_INSNS (38),           /* int_div_si */
     COSTS_N_INSNS (38),           /* int_div_di */
+    COSTS_N_INSNS (1),		  /* movz */
 		     2,           /* branch_cost */
 		     6            /* memory_latency */
   },
@@ -939,6 +959,7 @@ static const struct mips_rtx_cost_data
      COSTS_N_INSNS (10),          /* int_mult_di */
      COSTS_N_INSNS (69),          /* int_div_si */
      COSTS_N_INSNS (69),          /* int_div_di */
+     COSTS_N_INSNS (1),		  /* movz */
 		      2,          /* branch_cost */
 		      6           /* memory_latency */
   },
@@ -959,6 +980,7 @@ static const struct mips_rtx_cost_data
     COSTS_N_INSNS (6),            /* int_mult_di */
     COSTS_N_INSNS (69),           /* int_div_si */
     COSTS_N_INSNS (69),           /* int_div_di */
+    COSTS_N_INSNS (1),		  /* movz */
 		     1,           /* branch_cost */
 		     4            /* memory_latency */
   },
@@ -981,6 +1003,7 @@ static const struct mips_rtx_cost_data
     COSTS_N_INSNS (5),            /* int_mult_di */
     COSTS_N_INSNS (36),           /* int_div_si */
     COSTS_N_INSNS (36),           /* int_div_di */
+    COSTS_N_INSNS (1),		  /* movz */
 		     1,           /* branch_cost */
 		     4            /* memory_latency */
   },
@@ -994,6 +1017,7 @@ static const struct mips_rtx_cost_data
     COSTS_N_INSNS (4),            /* int_mult_di */
     COSTS_N_INSNS (42),           /* int_div_si */
     COSTS_N_INSNS (74),           /* int_div_di */
+    COSTS_N_INSNS (1),		  /* movz */
 		     1,           /* branch_cost */
 		     4            /* memory_latency */
   },
@@ -1007,6 +1031,7 @@ static const struct mips_rtx_cost_data
     COSTS_N_INSNS (9),            /* int_mult_di */
     COSTS_N_INSNS (42),           /* int_div_si */
     COSTS_N_INSNS (74),           /* int_div_di */
+    COSTS_N_INSNS (1),		  /* movz */
 		     1,           /* branch_cost */
 		     4            /* memory_latency */
   },
@@ -1022,6 +1047,7 @@ static const struct mips_rtx_cost_data
     COSTS_N_INSNS (9),            /* int_mult_di */
     COSTS_N_INSNS (69),           /* int_div_si */
     COSTS_N_INSNS (69),           /* int_div_di */
+    COSTS_N_INSNS (1),		  /* movz */
 		     1,           /* branch_cost */
 		     4            /* memory_latency */
   },
@@ -1040,6 +1066,7 @@ static const struct mips_rtx_cost_data
     COSTS_N_INSNS (8),            /* int_mult_di */
     COSTS_N_INSNS (69),           /* int_div_si */
     COSTS_N_INSNS (69),           /* int_div_di */
+    COSTS_N_INSNS (1),		  /* movz */
 		     1,           /* branch_cost */
 		     4            /* memory_latency */
   },
@@ -1053,6 +1080,7 @@ static const struct mips_rtx_cost_data
     COSTS_N_INSNS (9),            /* int_mult_di */
     COSTS_N_INSNS (34),           /* int_div_si */
     COSTS_N_INSNS (66),           /* int_div_di */
+    COSTS_N_INSNS (1),		  /* movz */
 		     1,           /* branch_cost */
 		     4            /* memory_latency */
   },
@@ -1067,6 +1095,7 @@ static const struct mips_rtx_cost_data
     COSTS_N_INSNS (4),            /* int_mult_di */
     COSTS_N_INSNS (36),           /* int_div_si */
     COSTS_N_INSNS (68),           /* int_div_di */
+    COSTS_N_INSNS (1),		  /* movz */
 		     1,           /* branch_cost */
 		     4            /* memory_latency */
   },
@@ -1081,6 +1110,7 @@ static const struct mips_rtx_cost_data
     COSTS_N_INSNS (4),            /* int_mult_di */
     COSTS_N_INSNS (36),           /* int_div_si */
     COSTS_N_INSNS (68),           /* int_div_di */
+    COSTS_N_INSNS (1),		  /* movz */
 		     1,           /* branch_cost */
 		     4            /* memory_latency */
   },
@@ -1093,6 +1123,7 @@ static const struct mips_rtx_cost_data
     COSTS_N_INSNS (8),            /* int_mult_di */
     COSTS_N_INSNS (72),           /* int_div_si */
     COSTS_N_INSNS (72),           /* int_div_di */
+    COSTS_N_INSNS (1),		  /* movz */
 		     1,           /* branch_cost */
 		     4            /* memory_latency */
   }
@@ -3941,6 +3972,10 @@ mips_rtx_costs (rtx x, int code, int outer_code, int opno ATTRIBUTE_UNUSED,
       *total = mips_cost->fp_add;
       return false;
 
+    case IF_THEN_ELSE:
+      *total = mips_cost->movz;
+      return false;
+
     default:
       return false;
     }

RE: [PATCH, ifcvt] Check size cost in noce_try_store_flag_mask

[Zhenqiang Chen]

Thank you all for the comments. Patch is updated.

Bootstrap and no make check regression on X86-64.
No make check regression with Cortex-M0 qemu.
No performance changes for coremark, dhrystone, spec2000 and spec2006 on
X86-64 and Cortex-A15.

For CSiBE, ARM Cortex-M0 result is a little better. A little regression for
MIPS (less than 0.01%).

diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index 653653f..7bb2578 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -1393,6 +1393,9 @@ noce_try_store_flag_mask (struct noce_if_info
*if_info)
 
       if (target)
 	{
+	  int old_cost, new_cost, insn_cost;
+	  int speed_p;
+
 	  if (target != if_info->x)
 	    noce_emit_move_insn (if_info->x, target);
 
@@ -1400,6 +1403,14 @@ noce_try_store_flag_mask (struct noce_if_info
*if_info)
 	  if (!seq)
 	    return FALSE;
 
+	  speed_p = optimize_bb_for_speed_p (BLOCK_FOR_INSN
(if_info->insn_a));
+	  insn_cost = insn_rtx_cost (PATTERN (if_info->insn_a), speed_p);
+	  old_cost = COSTS_N_INSNS (if_info->branch_cost) + insn_cost;
+	  new_cost = seq_cost (seq, speed_p);
+
+	  if (new_cost > old_cost)
+	    return FALSE;
+
 	  emit_insn_before_setloc (seq, if_info->jump,
 				   INSN_LOCATION (if_info->insn_a));
 	  return TRUE;
diff --git a/gcc/testsuite/gcc.target/arm/ifcvt-size-check.c
b/gcc/testsuite/gcc.target/arm/ifcvt-size-check.c
new file mode 100644
index 0000000..43fa16b
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/ifcvt-size-check.c
@@ -0,0 +1,13 @@
+/* { dg-do assemble } */
+/* { dg-options "-mthumb -Os " }  */
+/* { dg-require-effective-target arm_thumb1_ok } */
+
+int
+test (unsigned char iov_len, int count, int i) {
+  unsigned char bytes = 0;
+  if ((unsigned char) ((char) 127 - bytes) < iov_len)
+    return 22;
+  return 0;
+}
+/* { dg-final { object-size text <= 12 } } */


> -----Original Message-----
> From: Jeff Law [mailto:law@redhat.com]
> Sent: Thursday, October 30, 2014 1:27 PM
> To: Zhenqiang Chen; gcc-patches@gcc.gnu.org
> Subject: Re: [PATCH, ifcvt] Check size cost in noce_try_store_flag_mask
> 
> On 10/26/14 19:53, Zhenqiang Chen wrote:
> > Hi,
> >
> > Function noce_try_store_flag_mask converts "if (test) x = 0;" to "x &=
> > -(test == 0);"
> >
> > But from code size view, "x &= -(test == 0);" might have more
> > instructions than "if (test) x = 0;". The patch checks the cost to
> > determine the conversion is valuable or not.
> >
> > Bootstrap and no make check regression on X86-64.
> > No make check regression with Cortex-M0 qemu.
> > For CSiBE, ARM Cortex-m0 result is a little better. A little
> > regression for MIPS. Roughly no change for PowerPC.
> >
> > OK for trunk?
> >
> > Thanks!
> > -Zhenqiang
> >
> > ChangeLog:
> > 2014-10-27  Zhenqiang Chen  <zhenqiang.chen@arm.com>
> >
> > 	* ifcvt.c (noce_try_store_flag_mask): Check rtx cost.
> >
> > testsuite/ChangeLog:
> > 2014-10-27  Zhenqiang Chen  <zhenqiang.chen@arm.com>
> >
> > 	* gcc.target/arm/ifcvt-size-check.c: New test.
> >
> > diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c index 949d2b4..3abd518 100644
> > --- a/gcc/ifcvt.c
> > +++ b/gcc/ifcvt.c
> > @@ -1393,6 +1393,14 @@ noce_try_store_flag_mask (struct noce_if_info
> > *if_info)
> >   	  if (!seq)
> >   	    return FALSE;
> >
> > +	  if (optimize_function_for_size_p (cfun))
> > +	    {
> > +	      int old_cost = COSTS_N_INSNS (if_info->branch_cost + 1);
> > +	      int new_cost = seq_cost (seq, 0);
> > +	      if (new_cost > old_cost)
> > +		return FALSE;
> > +	    }
> > +
> As Andrew pointed out, there's really no reason to limit this to cases
where
> we're optimizing for size.  So that check should be removed.
> 
> You should also engage with Michael to determine if the MIPS regressions
> are significant enough to warrant deeper investigation.  My gut tells me
that
> if MIPS is regressing because of this, then that's going to be an issue in
the
> MIPS costing model that the MIPS folks will ultimately need to fix.
> 
> jeff
>