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

[Bernd Schmidt <bschmidt@redhat.com>]

On 09/29/2016 07:32 PM, Denys Vlasenko wrote:
> On 09/29/2016 04:45 PM, Bernd Schmidt wrote:
>> On 09/28/2016 02:57 PM, Denys Vlasenko wrote:
>>> -  /* 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.
>
> It would be awkward to translate -malign-loops=%d et al
> to comma-separated string format.
> Since this warning is there for some 15 years already,
> anyone who actually cares should have converted to new options
> long ago.

Hmm, if it's been 15 years, maybe it's time to remove these. Could you 
submit a patch separately?

>>> -  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.
>
> The execution flow for option parsing is somewhat convoluted, no doubt.
>
> I found it experimentally that these are locations where
> default alignment parameters are set when -falign-functions
> is given with no arguments (or when it is implied by -O2).

I applied your latest two patches to experiment with them, and I see 
different behaviour before and after on x86_64-linux. There seems to be 
a difference in function alignment and label alignment at -O2.

I think it would be good to add testcases first to document and verify 
existing behaviour, they would then serve to show whether it is 
unchanged afterwards.

Need to look into the x86 hook thing in a bit more detail.


Bernd