Re: avoid emitting function alignment twice

[Geoff Keating <geoffk at geoffk dot org>]

Dan Nicolaescu <dann@godzilla.ICS.UCI.EDU> writes:

> Geoff Keating <geoffk@geoffk.org> writes:
> 
>   > Dan Nicolaescu <dann@godzilla.ICS.UCI.EDU> writes:
>   > 
>   > > The following program: 
>   > > 
>   > > void f (void) {}
>   > > 
>   > > gets compiled to 
>   > > 
>   > > 	.file	"t.i"
>   > > 	.section	".text"
>   > > 	.align 4
>   > > 	.align 32   
>   > >         ^^^^^
>   > >        NOTE that .align occurs twice
>   > > 	.global f
>   > > 	.type	f,#function
>   > > [snip the rest, irrelevant]
>   > > 
>   > > on sun-sparc-solaris2.7 when using gcc -O2 -mtune=ultrasparc by both
>   > > the mainline and the 3.0 branch. 
>   > > 
>   > > gcc-2.95.2 only emits the second .align directive. 
>   > > 
>   > > Here is a patch that fixes this (bootstrapped on sun-sparc-solaris2.7
>   > > on the branch):
>   > > 
>   > > 2001-05-06  Dan Nicolaescu  <dann@ics.uci.edu>
>   > > 
>   > > 	* varasm.c (assemble_start_function): Avoid emitting alignment
>   > > 	twice.  
>   > 
>   > This is not OK.  It's possible that align_functions is less than
>   > FUNCTION_BOUNDARY.  It's also possible that the
>   > ASM_OUTPUT_MAX_SKIP_ALIGN will not actually align anything, because
>   > the alignment would require skipping too many bytes.
> 
> 
> Is it worth dealing with all these complications? 

Sure.  It's a valuable feature to be able to say 'I'd prefer to align
to a 32-byte boundary, but if it's going to involve skipping more than
23 bytes it's not worth it.'  This lets you get 75% of the speed
benefit at a cost of about 61% of the space.

> Is emitting .align twice harmless?

It has not caused any problems so far.

> Because if it is, then it might not
> be worth obfuscating the code to deal with it. If this is the case
> then maybe it would be useful to add a comment above the code stating
> that so somebody else does not make the same mistake that I
> made. (could you please do that, I don't have CVS access). 

The current sources already look like:

  /* Tell assembler to move to target machine's alignment for functions.  */
  align = floor_log2 (FUNCTION_BOUNDARY / BITS_PER_UNIT);
  if (align > 0)
    ASM_OUTPUT_ALIGN (asm_out_file, align);

  /* Handle a user-specified function alignment.
     Note that we still need to align to FUNCTION_BOUNDARY, as above,
     because ASM_OUTPUT_MAX_SKIP_ALIGN might not do any alignment at all.  */
  if (align_functions_log > align)
    {
#ifdef ASM_OUTPUT_MAX_SKIP_ALIGN
      ASM_OUTPUT_MAX_SKIP_ALIGN (asm_out_file, 
				 align_functions_log, align_functions-1);
#else
      ASM_OUTPUT_ALIGN (asm_out_file, align_functions_log);
#endif
    }

I can't think of any way to make the comment clearer, can you?

-- 
- Geoffrey Keating <geoffk@geoffk.org>