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

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

[Hu Jiangping]

Hi,

This patch deal with the -falign-X=0 options. According to man pages,
if zero is specified, a machine-dependent default value should be used.
But in fact, zero was used in internal process, it is inconsistent.

Tested on aarch64-linux cross compiler, Is that OK?

BTW, the similar problems exists in other target sources.
I can submit them all in another patch if needed,
but I can test on i386 target only.

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;
     }
 
-- 
2.17.1

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

[Hu, Jiangping]

PING.

> -----Original Message-----
> From: Gcc-patches <gcc-patches-bounces@gcc.gnu.org> On Behalf Of Hu
> Jiangping
> Sent: Tuesday, July 14, 2020 3:55 PM
> To: gcc-patches@gcc.gnu.org
> Subject: [PATCH] target: fix default value checking of x_str_align_functions in
> aarch64.c
> 
> Hi,
> 
> This patch deal with the -falign-X=0 options. According to man pages,
> if zero is specified, a machine-dependent default value should be used.
> But in fact, zero was used in internal process, it is inconsistent.
> 
> Tested on aarch64-linux cross compiler, Is that OK?
> 
> BTW, the similar problems exists in other target sources.
> I can submit them all in another patch if needed,
> but I can test on i386 target only.
> 
> 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;
>      }
> 
> --
> 2.17.1
> 
> 

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

[Richard Sandiford]

Hu Jiangping <hujiangping@cn.fujitsu.com> writes:
> Hi,
>
> This patch deal with the -falign-X=0 options. According to man pages,
> if zero is specified, a machine-dependent default value should be used.
> But in fact, zero was used in internal process, it is inconsistent.
>
> Tested on aarch64-linux cross compiler, Is that OK?
>
> BTW, the similar problems exists in other target sources.
> I can submit them all in another patch if needed,
> but I can test on i386 target only.

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.

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.

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;
>      }

RE: [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;
> >      }
> 

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

[Richard Sandiford]

"Hu, Jiangping" <hujiangping@cn.fujitsu.com> writes:
>> 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.

OK.

> 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.

Yeah, it isn't great.  On the other hand, it seems more natural for
-falign-functions=0 to be the same as -fno-align-functions than it is
for -falign-functions=1 to be the same as -fno-align-functions.

> 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.

Yeah, only saw that PR today, sorry.  Got a lot of email to catch
up on :-)

> 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?

Either's fine with me.  I guess treating =0 the same as =1 is less
likely to break existing Makefiles.  E.g. the usual practice has
been to keep old options that no longer have an effect, rather than
dropping them and turning them into errors.  On the other hand,
people trying to use -falign-functions=0 for the first time might
prefer an error instead of unexpected behaviour.

I guess yet another option would be to handle -falign-functions=0
like -falign-functions in common_handle_option (i.e. set the
x_str_align_functions to null and x_align_functions to true.)

Thanks,
Richard