[ifcvt] Check size cost in noce_try_store_flag_mask

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.

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 } } */
>
>
>

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 } } */
> 
>

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