Re: Revised mips alignment patches.

[Geoff Keating <geoffk@ozemail.com.au>]

> Date: Tue, 31 Aug 1999 12:09:56 -0700
> From: Richard Henderson <rth@cygnus.com>
> Cc: egcs-patches@gcc.gnu.org, Gavin Romig-Koch <gavin@cygnus.com>,
>         law@cygnus.com, davem@redhat.com
> 
> On Tue, Aug 31, 1999 at 04:40:08PM +1000, Geoff Keating wrote:
> > Also, all the implementations share a common bug: FUNCTION_BOUNDARY is
> > not just a request for an alignment, it's also an assertion that
> > function pointers have certain low bits clear (see
> > get_pointer_alignment in builtins.c and c_alignof in c-typeck.c).
> 
> Good eyes.
> 
> > I can easily make it -falign-functions=<n> if people think
> > that's an improvement, but we already have -finline-limit-<n>.
> 
> I'd much rather the `=' syntax, as it's more clear that the n is an
> argument.  I don't like -finline-limit-n or -fschedule-verbose-n either.

OK.  One recommendation is enough for me, it will have an '='.

Should I add support for -finline-limit=n etc. as well?

> > What do people think?
> 
> I think the patch is generally good.
> 
> > +  /* Set up the align_*_log variables, defaulting them to 1 if they
> > +     were still unset.  */
> > +  if (align_loops <= 0) align_loops = 1;
> > +  align_loops_log = floor_log2 (align_loops);
> > +  if (align_jumps <= 0) align_jumps = 1;
> > +  align_jumps_log = floor_log2 (align_jumps);
> > +  if (align_labels <= 0) align_labels = 1;
> > +  align_labels_log = floor_log2 (align_labels);
> > +  if (align_functions <= 0) align_functions = 1;
> > +  align_functions_log = floor_log2 (align_functions);
> 
> We should force align_functions to FUNCTION_BOUNDARY here, just
> so the back end's optimization_options doesn't have to, and ...
> 
> > +  /* 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.  */
> 
> ... we don't have to dual case this.

Unfortunately, the semantics of ASM_OUTPUT_MAX_SKIP_ALIGN is that it
either aligns it to the power of two that you asked for, or it does
_no alignment at all_ if the next boundary is too far.  So you have to
emit two alignment directives anyway.  I guess if
ASM_OUTPUT_MAX_SKIP_ALIGN was not defined, you'd avoid outputting two
.align directives, but this is not the usual case.

Also, it'd complicate the implementation of -fno-align-functions.

The back end doesn't need (in the current implementation) to change
any align_* to generate correct code, unless it has strange
requirements about how labels are aligned (eg. a machine with 32-bit
opcodes but with branches with 64-bit resolution).  They are purely
for optimisation.  I'd even suggest if alignment is needed for correct
code it'd be cleaner to define the LABEL_ALIGN macro rather than
change the align_* variables.

> > 	* config/mips/mips.c (override_options):  On 64-bit targets,
> > 	try to align things to 64-bit boundaries.
> > 	(print_operand): New substitution, %~,
> > 	which aligns labels to align_labels_log.
> > 	* config/mips/mips.md (div_trap_normal): Use %~.
> > 	(div_trap_mips16): Likewise.
> > 	(abssi): Likewise.
> > 	(absdi2): Likewise.
> > 	(ffssi2): Likewise.
> > 	(ffsdi2): Likewise.
> > 	(ashldi3_internal): Likewise.
> > 	(ashrdi3_internal): Likewise.
> > 	(lshrdi3_internal): Likewise.
> > 	(casesi_internal): Likewise.
> 
> Gavin should approve the mips stuff.

Yes.  He may have suggestions about what are the best default values
for MIPS.

> > 	(sparc_override_options): Default align_functions on ultrasparc.
> > 	Provide backwards compatibility for -malign-loops, -malign-jumps,
> > 	-malign-functions.
> 
> I don't see any point in providing backward compatibility on
> Sparc.  They aren't that heavily used except on x86, since they
> appear in the Linux kernel makefiles.
> 
> The Sparc bits are fine, otherwise.

OK, I'll delete the backwards-compatibility stuff and send in a new
patch (in case I delete too much :-).

-- 
Geoffrey Keating <geoffk@cygnus.com>