Disable -mbranch-likely for -Os when targetting generic architecture

> -----Original Message-----
> From: Robert Suchanek [mailto:Robert.Suchanek@imgtec.com]
> Sent: Friday, September 04, 2015 10:21 AM
> To: Matthew Fortune; Richard Sandiford
> Cc: Moore, Catherine; gcc-patches@gcc.gnu.org
> Subject: RE: [PATCH] Disable -mbranch-likely for -Os when targetting generic
> architecture
> 
> Hi,
> 
> > Richard Sandiford <rdsandiford@googlemail.com> writes:
> > > Robert Suchanek <Robert.Suchanek@imgtec.com> writes:
> > > > The patch below disables generation of the branch likely
> > > > instructions for -
> > Os
> > > > but only for generic architecture.  The branch likely may result
> > > > in some code size reduction but the cost of running the code on R6
> > > > core is
> > significant.
> > >
> > > How about instead splitting PTF_AVOID_BRANCHLIKELY into
> > > PTF_AVOID_BRANCHLIKELY_SPEED and
> PTF_AVOID_BRANCHLIKELY_SIZE?
> > > We could have PTF_AVOID_BRANCHLIKELY_ALWAYS as an OR of the two.
> >
> > This sounds OK and is nicer.
> >
> > > Anything that does string ops on the architecture is suspicious :-)
> >
> > You can blame me for this.  I advocated the string comparison approach
> > as I had to do the same thing in gas IIRC for some feature and
> > couldn't think of anything better to suggest.
> 
> Here is an updated version that use the suggested method. Ok to apply?
> 
> Regards,
> Robert
> 
> gcc/
> 	* config/mips/mips-cpus.def: Replace PTF_AVOID_BRANCHLIKELY
> with
> 	PTF_AVOID_BRANCHLIKELY_ALWAYS for generic architecture and
> with
> 	PTF_AVOID_BRANCHLIKELY_SPEED for others.
> 	(mips2, mips3, mips4): Add PTF_AVOID_BRANCHLIKELY_SIZE to tune
> flags.
> 	* config/mips/mips.c (mips_option_override): Enable the branch
> likely
> 	depending on the tune flags and optimization level.
> 	* config/mips/mips.h (PTF_AVOID_BRANCHLIKELY): Remove.
> 	(PTF_AVOID_BRANCHLIKELY_SPEED): Define.
> 	(PTF_AVOID_BRANCHLIKELY_SIZE): Likewise.
> 	(PTF_AVOID_BRANCHLIKELY_ALWAYS): Likewise.
> ---
>  gcc/config/mips/mips-cpus.def | 56 +++++++++++++++++++++---------------
> -------
>  gcc/config/mips/mips.c        |  6 +++--
>  gcc/config/mips/mips.h        | 20 ++++++++++++----
>  3 files changed, 47 insertions(+), 35 deletions(-)
> 
> a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c index 0e0ecf2..f8775c4
> 100644
> --- a/gcc/config/mips/mips.c
> +++ b/gcc/config/mips/mips.c
> @@ -17916,8 +17916,10 @@ mips_option_override (void)
>    if ((target_flags_explicit & MASK_BRANCHLIKELY) == 0)
>      {
>        if (ISA_HAS_BRANCHLIKELY
> -	  && (optimize_size
> -	      || (mips_tune_info->tune_flags & PTF_AVOID_BRANCHLIKELY)
> == 0))
> +	  && ((optimize_size && (mips_tune_info->tune_flags
> +				 & PTF_AVOID_BRANCHLIKELY_SIZE) == 0)
> +	       || (!optimize_size && (mips_tune_info->tune_flags
> +				      & PTF_AVOID_BRANCHLIKELY_SPEED) ==
> 0)))
>  	target_flags |= MASK_BRANCHLIKELY;
>        else
>  	target_flags &= ~MASK_BRANCHLIKELY;

Should this check be:

Instead?  I don't see a use of PTF_AVOID_BRANCH_ALWAYS in your patch, but it seems like it should be checked.

Hi Catherine,

Apologies for the (very) late reply.
It appears that I never replied to the last message.

> > gcc/
> > 	* config/mips/mips-cpus.def: Replace PTF_AVOID_BRANCHLIKELY
> > with
> > 	PTF_AVOID_BRANCHLIKELY_ALWAYS for generic architecture and
> > with
> > 	PTF_AVOID_BRANCHLIKELY_SPEED for others.
> > 	(mips2, mips3, mips4): Add PTF_AVOID_BRANCHLIKELY_SIZE to tune
> > flags.
> > 	* config/mips/mips.c (mips_option_override): Enable the branch
> > likely
> > 	depending on the tune flags and optimization level.
> > 	* config/mips/mips.h (PTF_AVOID_BRANCHLIKELY): Remove.
> > 	(PTF_AVOID_BRANCHLIKELY_SPEED): Define.
> > 	(PTF_AVOID_BRANCHLIKELY_SIZE): Likewise.
> > 	(PTF_AVOID_BRANCHLIKELY_ALWAYS): Likewise.
> > ---
> >  gcc/config/mips/mips-cpus.def | 56 +++++++++++++++++++++---------------
> > -------
> >  gcc/config/mips/mips.c        |  6 +++--
> >  gcc/config/mips/mips.h        | 20 ++++++++++++----
> >  3 files changed, 47 insertions(+), 35 deletions(-)
> >
> > a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c index 0e0ecf2..f8775c4
> > 100644
> > --- a/gcc/config/mips/mips.c
> > +++ b/gcc/config/mips/mips.c
> > @@ -17916,8 +17916,10 @@ mips_option_override (void)
> >    if ((target_flags_explicit & MASK_BRANCHLIKELY) == 0)
> >      {
> >        if (ISA_HAS_BRANCHLIKELY
> > -	  && (optimize_size
> > -	      || (mips_tune_info->tune_flags & PTF_AVOID_BRANCHLIKELY)
> > == 0))
> > +	  && ((optimize_size && (mips_tune_info->tune_flags
> > +				 & PTF_AVOID_BRANCHLIKELY_SIZE) == 0)
> > +	       || (!optimize_size && (mips_tune_info->tune_flags
> > +				      & PTF_AVOID_BRANCHLIKELY_SPEED) ==
> > 0)))
> >  	target_flags |= MASK_BRANCHLIKELY;
> >        else
> >  	target_flags &= ~MASK_BRANCHLIKELY;
> 
> Should this check be:
> Index: mips.c
> ===================================================================
> --- mips.c      (revision 229138)
> +++ mips.c      (working copy)
> @@ -17758,8 +17758,15 @@
>    if ((target_flags_explicit & MASK_BRANCHLIKELY) == 0)
>      {
>        if (ISA_HAS_BRANCHLIKELY
> -         && (optimize_size
> -             || (mips_tune_info->tune_flags & PTF_AVOID_BRANCHLIKELY) == 0))
> +         && ((optimize_size
> +              && (mips_tune_info->tune_flags
> +                  & PTF_AVOID_BRANCHLIKELY_SIZE) == 0)
> +             || (!optimize_size
> +                && optimize > 0
> +                && ((mips_tune_info->tune_flags
> +                     & PTF_AVOID_BRANCHLIKELY_SPEED) == 0))
> +            || (mips_tune_info->tune_flags
> +                 & PTF_AVOID_BRANCHLIKELY_ALWAYS) == 0))
>         target_flags |= MASK_BRANCHLIKELY;
>        else
>         target_flags &= ~MASK_BRANCHLIKELY;
> 
> Instead?  I don't see a use of PTF_AVOID_BRANCH_ALWAYS in your patch, but it
> seems like it should be checked.
> 

I did that on purpose at the time as the check looked redundant as it will be one
or the other.  However, for easier reading and a potential redefinition of *_ALWAYS
e.g. to a unique value then the extra check is a must.

I'm happy to include this.  Ok to commit with this change?

Regards,
Robert

Robert Suchanek <Robert.Suchanek@imgtec.com> writes:
> Hi Catherine,
> 
> Apologies for the (very) late reply.
> It appears that I never replied to the last message.
> 
> > > gcc/
> > > 	* config/mips/mips-cpus.def: Replace PTF_AVOID_BRANCHLIKELY with
> > > 	PTF_AVOID_BRANCHLIKELY_ALWAYS for generic architecture and with
> > > 	PTF_AVOID_BRANCHLIKELY_SPEED for others.
> > > 	(mips2, mips3, mips4): Add PTF_AVOID_BRANCHLIKELY_SIZE to tune
> > > flags.
> > > 	* config/mips/mips.c (mips_option_override): Enable the branch
> > > likely
> > > 	depending on the tune flags and optimization level.
> > > 	* config/mips/mips.h (PTF_AVOID_BRANCHLIKELY): Remove.
> > > 	(PTF_AVOID_BRANCHLIKELY_SPEED): Define.
> > > 	(PTF_AVOID_BRANCHLIKELY_SIZE): Likewise.
> > > 	(PTF_AVOID_BRANCHLIKELY_ALWAYS): Likewise.
> > > ---
> > >  gcc/config/mips/mips-cpus.def | 56
> > > +++++++++++++++++++++---------------
> > > -------
> > >  gcc/config/mips/mips.c        |  6 +++--
> > >  gcc/config/mips/mips.h        | 20 ++++++++++++----
> > >  3 files changed, 47 insertions(+), 35 deletions(-)
> > >
> > > a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c index
> > > 0e0ecf2..f8775c4
> > > 100644
> > > --- a/gcc/config/mips/mips.c
> > > +++ b/gcc/config/mips/mips.c
> > > @@ -17916,8 +17916,10 @@ mips_option_override (void)
> > >    if ((target_flags_explicit & MASK_BRANCHLIKELY) == 0)
> > >      {
> > >        if (ISA_HAS_BRANCHLIKELY
> > > -	  && (optimize_size
> > > -	      || (mips_tune_info->tune_flags & PTF_AVOID_BRANCHLIKELY)
> > > == 0))
> > > +	  && ((optimize_size && (mips_tune_info->tune_flags
> > > +				 & PTF_AVOID_BRANCHLIKELY_SIZE) == 0)
> > > +	       || (!optimize_size && (mips_tune_info->tune_flags
> > > +				      & PTF_AVOID_BRANCHLIKELY_SPEED) ==
> > > 0)))
> > >  	target_flags |= MASK_BRANCHLIKELY;
> > >        else
> > >  	target_flags &= ~MASK_BRANCHLIKELY;
> >
> > Should this check be:
> > Index: mips.c
> > ===================================================================
> > --- mips.c      (revision 229138)
> > +++ mips.c      (working copy)
> > @@ -17758,8 +17758,15 @@
> >    if ((target_flags_explicit & MASK_BRANCHLIKELY) == 0)
> >      {
> >        if (ISA_HAS_BRANCHLIKELY
> > -         && (optimize_size
> > -             || (mips_tune_info->tune_flags & PTF_AVOID_BRANCHLIKELY)
> == 0))
> > +         && ((optimize_size
> > +              && (mips_tune_info->tune_flags
> > +                  & PTF_AVOID_BRANCHLIKELY_SIZE) == 0)
> > +             || (!optimize_size
> > +                && optimize > 0
> > +                && ((mips_tune_info->tune_flags
> > +                     & PTF_AVOID_BRANCHLIKELY_SPEED) == 0))
> > +            || (mips_tune_info->tune_flags
> > +                 & PTF_AVOID_BRANCHLIKELY_ALWAYS) == 0))
> >         target_flags |= MASK_BRANCHLIKELY;
> >        else
> >         target_flags &= ~MASK_BRANCHLIKELY;
> >
> > Instead?  I don't see a use of PTF_AVOID_BRANCH_ALWAYS in your patch,
> > but it seems like it should be checked.
> >
> 
> I did that on purpose at the time as the check looked redundant as it
> will be one or the other.  However, for easier reading and a potential
> redefinition of *_ALWAYS e.g. to a unique value then the extra check is
> a must.
> 
> I'm happy to include this.  Ok to commit with this change?

This looks like it got lost at some point. I think this is a reasonable
change for safety.

Go ahead and commit.

Thanks,
Matthew

Hi,

> > I'm happy to include this.  Ok to commit with this change?
> 
> This looks like it got lost at some point. I think this is a reasonable
> change for safety.
> 
> Go ahead and commit.
> 
> Thanks,
> Matthew

Committed as r240965.

Regards,
Robert