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

[Denys Vlasenko <dvlasenk@redhat.com>]

On 09/30/2016 01:20 PM, Bernd Schmidt wrote:
> 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?

Sure.

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

Let me try harder, I was only checking -ffunction-alignment...

My test program:

int g();
int f(int i)
{
         i *= 3;
         while (--i > 100) {
  L1:
                 if (g())
                         goto L1;
                 if (g())
                         goto L2;
         }
         return i;
  L2:
         return 123;
}

Before-and-after "gcc -O2 -S" assembly (after the patch is on the right):

         .text                                   .text
         .p2align 4,,15                          .p2align 4,,15
         .globl  f                               .p2align 3
         .type   f, @function                    .globl  f
f:                                              .type   f, @function
.LFB0:                                  f:
         .cfi_startproc                  .LFB0:
         pushq   %rbx                            .cfi_startproc
         .cfi_def_cfa_offset 16                  pushq   %rbx
         .cfi_offset 3, -16                      .cfi_def_cfa_offset 16
         leal    (%rdi,%rdi,2), %ebx             .cfi_offset 3, -16
         .p2align 4,,10                          leal    (%rdi,%rdi,2), %ebx
         .p2align 3                              .p2align 4,,10
.L2:                                    .L2:
         subl    $1, %ebx                        subl    $1, %ebx
         cmpl    $100, %ebx                      cmpl    $100, %ebx
         jle     .L1                             jle     .L1
         .p2align 4,,10                          .p2align 4,,10
         .p2align 3                      .L3:
.L3:                                            xorl    %eax, %eax
         xorl    %eax, %eax                      call    g
         call    g                               testl   %eax, %eax
         testl   %eax, %eax                      jne     .L3
         jne     .L3                             call    g
         call    g                               testl   %eax, %eax
         testl   %eax, %eax                      je      .L2
         je      .L2                             movl    $123, %ebx
         movl    $123, %ebx              .L4:
.L4:                                    .L1:
.L1:                                            movl    %ebx, %eax
         movl    %ebx, %eax                      popq    %rbx
         popq    %rbx                            .cfi_def_cfa_offset 8
         .cfi_def_cfa_offset 8                   ret
         ret                                     .cfi_endproc
         .cfi_endproc                    .LFE0:
.LFE0:


Yes, I see differences. ".p2align 3" appeared in function alignment.
The reason is that old code had an optimization - it noticed that
".p2align 4,,15" _always_ aligns (because 2^4=15+1), thus ".p2align 3"
is superfluous. My patch doesn't do that. I fixed this already
in the next version of the patch I'm going to send.

The other difference is that ".p2align 4,,10" is no longer followed
by ".p2align 3". Well... this one is harder to make happen.
It comes from here in gcc/final.c:

     case CODE_LABEL:
       /* The target port might emit labels in the output function for
          some insn, e.g. sh.c output_branchy_insn.  */
       if (CODE_LABEL_NUMBER (insn) <= max_labelno)
         {
           int align = LABEL_TO_ALIGNMENT (insn);
#ifdef ASM_OUTPUT_MAX_SKIP_ALIGN
           int max_skip = LABEL_TO_MAX_SKIP (insn);
#endif

           if (align && NEXT_INSN (insn))
             {
#ifdef ASM_OUTPUT_MAX_SKIP_ALIGN
               ASM_OUTPUT_MAX_SKIP_ALIGN (file, align, max_skip);  <===HERE
#else


The difficulty is that (align, max_skip) tuple here is arrived at
a rather complex process. It's not a label, jump or loop alignment,
it can even be neither of them (rs6000 has an override).

I am inclined to do this:

               ASM_OUTPUT_MAX_SKIP_ALIGN (file, align, max_skip);
               /* Above, we don't know whether a label, jump or loop
                  alignment was used. Conservatively apply
                  label subalignment if any, not jump or loop
                  subalignment (they are almost always larger).  */
               ASM_OUTPUT_MAX_SKIP_ALIGN (file, align_labels[1].log,
                                          align_labels[1].maxskip);

With default -O2 parameters, this still wouldn't match the previous
behavior: the default label alignment is 0, so implied subalignment
is 0 too. I'll try to tweak processor_target_table[] and let you know
how it goes.

The big way of handling this would be to stop carrying separate
(log, maxskip) pairs around, pass a pointer to struct align_flags[2]
instead which contains two pairs of (log,maxskip) tuples.