[PATCH] target: fix default value checking of x_str_align_functions in aarch64.c

[Hu, Jiangping]

> Sorry for the slow response on this.  Like you say, it seems to be a pretty
> pervasive problem.  In fact I couldn't see anywhere that actually treated -
> falign-foo=0 as anything other than -falign-foo=1.
> 
> Technically using an alignment of one for zero is within what the
> documentation allows, but not in a useful way.  The documentation also isn't
> clear about whether:
> 
>   -falign-loops=0:8
> 
> (“align to whatever you think is best, but don't skip more than 8 bytes”) is
> supposed to be valid.  The implication is that it's OK, but in practice it doesn't
> work.
> 
Thanks, Richard!

> If there isn't anywhere that handles zero in the way that the documentation
> implies (i.e. with -falign-loops=0 being equivalent to -falign-loops) then maybe
> we should instead change the documentation to match the actual behaviour.
>
Yes, I confirmed in source level that there is no target that handles zero in the way that the documentation implies.
But, I think just adjusting the documentation to reflect the implementation behavior will cause -falign-functions=0 and -falign-functions=1 to have the same meaning which is a bit confusing from the user's perspective.

In fact, I have issued a pr to Bugzilla, and got a comment from Richard Biener. We are considering whether to reject 0, which means both the source and the documentation should be modified a bit.
I found that function parse_and_check_align_values in gcc/opts.c maybe the point we want to modify, values less than 0 are currently rejected here. Maybe we can change the '<’ to '<=' in the if statement. But I need  test, to make sure there are no regression issues.

What do you think?

Regards!
Hujp

> If instead this is a regression from previous compilers, then I guess we should
> fix it.  But I think it would be good to have a helper function that tests whether
> the default should be used for a given x_flag_align_foo and x_str_align_foo
> pair.  That we we could reuse it in other targets and would have only one place
> to update.  (For example, we might decide to use
> parse_and_check_align_values rather than strcmp.)
> 
> Don't know whether anyone else has any thoughts about the best fix.
> 
> Thanks, and sorry again for the slow reply.
> 
> Richard
> 
> >
> > Regards!
> > Hujp
> >
> > ---
> >  gcc/config/aarch64/aarch64.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/gcc/config/aarch64/aarch64.c
> > b/gcc/config/aarch64/aarch64.c index 17dbe673978..697ac676f4d
> 100644
> > --- a/gcc/config/aarch64/aarch64.c
> > +++ b/gcc/config/aarch64/aarch64.c
> > @@ -14221,11 +14221,14 @@ aarch64_override_options_after_change_1
> (struct gcc_options *opts)
> >       alignment to what the target wants.  */
> >    if (!opts->x_optimize_size)
> >      {
> > -      if (opts->x_flag_align_loops && !opts->x_str_align_loops)
> > +      if ((opts->x_flag_align_loops && !opts->x_str_align_loops)
> > +        || (opts->x_str_align_loops &&
> > + strcmp(opts->x_str_align_loops, "0") == 0))
> >  	opts->x_str_align_loops = aarch64_tune_params.loop_align;
> > -      if (opts->x_flag_align_jumps && !opts->x_str_align_jumps)
> > +      if ((opts->x_flag_align_jumps && !opts->x_str_align_jumps)
> > +        || (opts->x_str_align_jumps &&
> > + strcmp(opts->x_str_align_jumps, "0") == 0))
> >  	opts->x_str_align_jumps = aarch64_tune_params.jump_align;
> > -      if (opts->x_flag_align_functions && !opts->x_str_align_functions)
> > +      if ((opts->x_flag_align_functions && !opts->x_str_align_functions)
> > +        || (opts->x_str_align_functions &&
> > + strcmp(opts->x_str_align_functions, "0") == 0))
> >  	opts->x_str_align_functions = aarch64_tune_params.function_align;
> >      }
>