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

[Richard Sandiford <richard.sandiford@arm.com>]

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