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

["Zhenqiang Chen" <zhenqiang dot chen at arm dot com>]

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
>