Re: [PATCH 2/2] Extend -falign-FOO=N to N[,M[,N2[,M2]]]

[Bernd Schmidt <bschmidt at redhat dot com>]

On 09/28/2016 02:57 PM, Denys Vlasenko wrote:

> This change makes it possible to align functions to 64-byte boundaries *if*
> this does not introduce huge amount of padding.

I'm still somewhat undecided, but it seems like a decent enough way to 
expose these possibilities to the user, so unless someone objects, I 
think I'll approve the final version. But first, there are a few more 
things to address (you might want to read the patch submission 
guidelines on our web pages).

> Testing:

We require that every patch is bootstrapped and the regression testsuite 
run on at least one primary target. Since you're modifying several 
targets it would be good to at least build them as well. Patch 
submission mails need to state that such testing happened and on which 
targets.

> Tested that with -falign-functions=N (tried 8, 15, 16, 17...) the alignment
> directives are the same before and after the patch.
> Tested that -falign-functions=N,N (two equal parameters) works exactly
> like -falign-functions=N.
>
> No change from past behavior:
> Tested that "-falign-functions" uses an arch-dependent alignment.
> Tested that "-O2" uses an arch-dependent alignment.
> Tested that "-O2 -falign-functions=N" uses explicitly given alignment.

I suspect we may want some testcases to cover this as well as the new 
functionality. Look for existing ones that can maybe be adapted.

> Index: gcc/common/config/i386/i386-common.c
> ===================================================================
> --- gcc/common/config/i386/i386-common.c	(revision 239860)
> +++ gcc/common/config/i386/i386-common.c	(working copy)
> @@ -998,36 +998,17 @@ ix86_handle_option (struct gcc_options *opts,
>  	}
>        return true;
>
> -
> -  /* Comes from final.c -- no real reason to change it.  */
> -#define MAX_CODE_ALIGN 16
> -
>      case OPT_malign_loops_:
>        warning_at (loc, 0, "-malign-loops is obsolete, use -falign-loops");
> -      if (value > MAX_CODE_ALIGN)
> -	error_at (loc, "-malign-loops=%d is not between 0 and %d",
> -		  value, MAX_CODE_ALIGN);
> -      else
> -	opts->x_align_loops = 1 << value;
>        return true;

That does seem to be a functional change. I'll defer to Uros.

> Index: gcc/config/arm/arm.c
> ===================================================================
> --- gcc/config/arm/arm.c	(revision 239860)
> +++ gcc/config/arm/arm.c	(working copy)
> @@ -2899,9 +2899,10 @@ static GTY(()) tree init_optimize;
>  static void
>  arm_override_options_after_change_1 (struct gcc_options *opts)
>  {
> -  if (opts->x_align_functions <= 0)
> +  if (opts->x_flag_align_functions && !opts->x_str_align_functions)

Are these conditions really equivalent? It looks like zero was the 
default even when no -falign-functions was specified. Or is that 
overriden by init_alignments?

>  {
> -  if (opts->x_align_loops == 0)
> +  /* -falign-foo without argument: supply one */
> +  if (opts->x_flag_align_loops && !opts->x_str_align_loops)

Same here.

> +@gccoptlist{-faggressive-loop-optimizations @gol
> +-falign-functions[=@var{n}[,@var{m},[@var{n}[,@var{m}]]]] @gol
> +-falign-jumps[=@var{n}[,@var{m},[@var{n}[,@var{m}]]]] @gol
> +-falign-labels[=@var{n}[,@var{m},[@var{n}[,@var{m}]]]] @gol
> +-falign-loops[=@var{n}[,@var{m},[@var{n}[,@var{m}]]]] @gol

I think it would be best not to use the same name for different 
arguments. Maybe call the second set n2, m2 (everywhere).

>  @item -falign-functions
>  @itemx -falign-functions=@var{n}
> +@itemx -falign-functions=@var{n},@var{m}
> +@itemx -falign-functions=@var{n},@var{m},@var{n},@var{m}

Three args is also valid, isn't it (here and for the other options)?

> +If @var{m} is not specified, it defaults to @var{n}.

I'd move these notes after the point where M is explained, in all places 
where they occur.

>  @item -falign-labels
>  @itemx -falign-labels=@var{n}
> +@itemx -falign-labels=@var{n},@var{m}
> +@itemx -falign-labels=@var{n},@var{m},@var{n},@var{m}
>  @opindex falign-labels
> +If @var{m} is not specified, it defaults to @var{n}.
>  Align all branch targets to a power-of-two boundary, skipping up to
> -@var{n} bytes like @option{-falign-functions}.  This option can easily
> +@var{m}-1 bytes like @option{-falign-functions}.

Maybe just write "Align all branch targets to a power-of-two boundary. 
The options are exactly as described for @option{-falign-functions}."

Why m-1, by the way, Wouldn't it be simpler to just specify the maximum 
number of bytes skipped?

> ===================================================================
> --- gcc/toplev.c	(revision 239860)
> +++ gcc/toplev.c	(working copy)
> @@ -1177,29 +1177,78 @@ target_supports_section_anchors_p (void)
>    return true;
>  }
>
> -/* Default the align_* variables to 1 if they're still unset, and
> -   set up the align_*_log variables.  */
> +static const char *
> +read_uint (const char *flag, const char *name, int *np)

Every function should have a comment in front of it that documents the 
arguments and the return value (argument names in caps). This is as 
described in the GNU coding standards.

> +  if (m > 0) m--; /* -falign-foo=N,M means M-1 max bytes of padding, not M */

See above - why not specify the real max?

> +#ifdef SUBALIGN_LOG

We want to avoid adding new #defines; existing ones are being converted 
to target hooks. I suspect the best way is to record whether an M value 
was specified, and override it in the x86 option_override if required. 
If that's infeasible for some reason we can revisit this.

> +      /* Before -falign-foo=N,M,N2,M2 was introduced, x86 had a tweak.
> +	 -falign-functions=N with N > 8 was adding secondary alignment.
> +	 -falign-functions=10 was emitting this before every function:
> +		.p2align 4,,9
> +		.p2align 3
> +	 Now this behavior (and more) can be explicitly requested:
> +	 -falign-functions=16,10,8
> +	 Retain old behavior if N2 is missing: */
> +      else if (a[0].log > SUBALIGN_LOG)
> +	a[1].log = SUBALIGN_LOG;

So this would live in i386.c, probably.


Bernd