Re: [PATCH] Avoid crtl->stack_realign_needed just because of DImode/long long pseudos or arguments (PR rtl-optimization/40667)

[Richard Guenther <richard dot guenther at gmail dot com>]

On Fri, Jul 10, 2009 at 2:33 PM, Jakub Jelinek<jakub@redhat.com> wrote:
> Hi!
>
> Currently GCC forces frame pointer with
> -m32 -mpreferred-stack-boundary=2 -fomit-frame-pointer
> whenever long long argument is seen, or even just external or static
> long long variable is referenced in a function.
> The problem is in a couple of places which bump
> crtl->stack_alignment_estimated to 64.
>
> The following patch changes those spots not to do that if the object doesn't
> really require it.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk/4.4?
>
> 2009-07-10 ?Jakub Jelinek ?<jakub@redhat.com>
>
> ? ? ? ?PR rtl-optimization/40667
> ? ? ? ?* defaults.h (MINIMUM_ALIGNMENT): Define if not defined.
> ? ? ? ?* doc/tm.texi (MINIMUM_ALIGNMENT): Document it.
> ? ? ? ?* config/i386/i386.h (MINIMUM_ALIGNMENT): Define.
> ? ? ? ?* config/i386/i386.c (ix86_minimum_alignment): New function.
> ? ? ? ?* config/i386/i386-protos.h (ix86_minimum_alignment): New prototype.
> ? ? ? ?* cfgexpand.c (expand_one_var): Use MINIMIM_ALIGNMENT.
> ? ? ? ?* emit-rtl.c (gen_reg_rtx): Likewise.
> ? ? ? ?* function.c (assign_parms): Likewise. ?If nominal_type needs
> ? ? ? ?bigger alignment than FUNCTION_ARG_BOUNDARY, use its alignment
> ? ? ? ?rather than passed_type's alignment.
>
> --- gcc/defaults.h.jj ? 2009-06-30 13:10:29.000000000 +0200
> +++ gcc/defaults.h ? ? ?2009-07-10 12:10:49.000000000 +0200
> @@ -1138,6 +1138,10 @@ see the files COPYING3 and COPYING.RUNTI
> ? LOCAL_ALIGNMENT (TREE_TYPE (DECL), DECL_ALIGN (DECL))
> ?#endif
>
> +#ifndef MINIMUM_ALIGNMENT
> +#define MINIMUM_ALIGNMENT(EXP,MODE,ALIGN) (ALIGN)
> +#endif
> +
> ?/* Alignment value for attribute ((aligned)). ?*/
> ?#ifndef ATTRIBUTE_ALIGNED_VALUE
> ?#define ATTRIBUTE_ALIGNED_VALUE BIGGEST_ALIGNMENT
> --- gcc/doc/tm.texi.jj ?2009-06-30 13:09:49.000000000 +0200
> +++ gcc/doc/tm.texi ? ? 2009-07-10 12:15:04.000000000 +0200
> @@ -1227,6 +1227,14 @@ One use of this macro is to increase ali
> ?make it all fit in fewer cache lines.
> ?@end defmac
>
> +@defmac MINIMUM_ALIGNMENT (@var{exp}, @var{mode}, @var{align})
> +If defined, a C expression to compute the minimum required alignment
> +for dynamic stack realignment purposes for @var{exp} (a type or decl),
> +@var{mode}, assuming normal alignment @var{align}.
> +
> +If this macro is not defined, then @var{align} will be used.
> +@end defmac
> +
> ?@defmac EMPTY_FIELD_BOUNDARY
> ?Alignment in bits to be given to a structure bit-field that follows an
> ?empty field such as @code{int : 0;}.
> --- gcc/config/i386/i386.h.jj ? 2009-07-08 23:36:32.000000000 +0200
> +++ gcc/config/i386/i386.h ? ? ?2009-07-10 12:09:28.000000000 +0200
> @@ -831,6 +831,15 @@ enum target_cpu_default
> ?#define LOCAL_DECL_ALIGNMENT(DECL) \
> ? ix86_local_alignment ((DECL), VOIDmode, DECL_ALIGN (DECL))
>
> +/* If defined, a C expression to compute the minimum required alignment
> + ? for dynamic stack realignment purposes for EXP (a TYPE or DECL),
> + ? MODE, assuming normal alignment ALIGN.
> +
> + ? If this macro is not defined, then (ALIGN) will be used. ?*/
> +
> +#define MINIMUM_ALIGNMENT(EXP, MODE, ALIGN) \
> + ?ix86_minimum_alignment (EXP, MODE, ALIGN)
> +
>
> ?/* If defined, a C expression that gives the alignment boundary, in
> ? ?bits, of an argument with the specified mode and type. ?If it is
> --- gcc/config/i386/i386.c.jj ? 2009-07-08 23:36:33.000000000 +0200
> +++ gcc/config/i386/i386.c ? ? ?2009-07-10 12:09:18.000000000 +0200
> @@ -20087,6 +20087,41 @@ ix86_local_alignment (tree exp, enum mac
> ? ? }
> ? return align;
> ?}
> +
> +/* Compute the minimum required alignment for dynamic stack realignment
> + ? purposes for a local variable, parameter or a stack slot. ?EXP is
> + ? the data type or decl itself, MODE is its mode and ALIGN is the
> + ? alignment that the object would ordinarily have. ?*/
> +
> +unsigned int
> +ix86_minimum_alignment (tree exp, enum machine_mode mode,
> + ? ? ? ? ? ? ? ? ? ? ? unsigned int align)
> +{
> + ?tree type, decl;
> +
> + ?if (TARGET_64BIT || align != 64 || ix86_preferred_stack_boundary >= 64)
> + ? ?return align;
> +
> + ?if (exp && DECL_P (exp))
> + ? ?{
> + ? ? ?type = TREE_TYPE (exp);
> + ? ? ?decl = exp;
> + ? ?}
> + ?else
> + ? ?{
> + ? ? ?type = exp;
> + ? ? ?decl = NULL;
> + ? ?}
> +
> + ?/* Don't do dynamic stack realignment for long long objects with
> + ? ? -mpreferred-stack-boundary=2. ?*/
> + ?if ((mode == DImode || (type && TYPE_MODE (type) == DImode))

What about DFmode?

> + ? ? ?&& (!type || !TYPE_USER_ALIGN (type))
> + ? ? ?&& (!decl || !DECL_USER_ALIGN (decl)))
> + ? ?return 32;
> +
> + ?return align;
> +}
>
> ?/* Emit RTL insns to initialize the variable parts of a trampoline.
> ? ?FNADDR is an RTX for the address of the function's pure code.
> --- gcc/config/i386/i386-protos.h.jj ? ?2009-07-08 23:36:33.000000000 +0200
> +++ gcc/config/i386/i386-protos.h ? ? ? 2009-07-10 12:04:13.000000000 +0200
> @@ -199,6 +199,8 @@ extern int ix86_return_pops_args (tree,
> ?extern int ix86_data_alignment (tree, int);
> ?extern unsigned int ix86_local_alignment (tree, enum machine_mode,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?unsigned int);
> +extern unsigned int ix86_minimum_alignment (tree, enum machine_mode,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? unsigned int);
> ?extern int ix86_constant_alignment (tree, int);
> ?extern tree ix86_handle_shared_attribute (tree *, tree, tree, int, bool *);
> ?extern tree ix86_handle_selectany_attribute (tree *, tree, tree, int, bool *);
> --- gcc/cfgexpand.c.jj ?2009-06-30 13:10:30.000000000 +0200
> +++ gcc/cfgexpand.c ? ? 2009-07-10 12:11:40.000000000 +0200
> @@ -1164,9 +1164,11 @@ expand_one_var (tree var, bool toplevel,
> ? ? ? ? variables, which won't be on stack, we collect alignment of
> ? ? ? ? type and ignore user specified alignment. ?*/
> ? ? ? if (TREE_STATIC (var) || DECL_EXTERNAL (var))
> - ? ? ? align = TYPE_ALIGN (TREE_TYPE (var));
> + ? ? ? align = MINIMUM_ALIGNMENT (TREE_TYPE (var),
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?TYPE_MODE (TREE_TYPE (var)),
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?TYPE_ALIGN (TREE_TYPE (var)));

static or external vars shouldn't use MINIMUM_ALIGNMENT which
is documented as for stack re-alignment purposes.

The rest of the patch looks ok.  Please leave some time for x86
backend maintainers to comment.

Thanks,
Richard.

> ? ? ? else
> - ? ? ? align = DECL_ALIGN (var);
> + ? ? ? align = MINIMUM_ALIGNMENT (var, DECL_MODE (var), DECL_ALIGN (var));
>
> ? ? ? if (crtl->stack_alignment_estimated < align)
> ? ? ? ? {
> --- gcc/emit-rtl.c.jj ? 2009-06-30 13:10:30.000000000 +0200
> +++ gcc/emit-rtl.c ? ? ?2009-07-10 12:12:35.000000000 +0200
> @@ -869,7 +869,11 @@ gen_reg_rtx (enum machine_mode mode)
> ? if (SUPPORTS_STACK_ALIGNMENT
> ? ? ? && crtl->stack_alignment_estimated < align
> ? ? ? && !crtl->stack_realign_processed)
> - ? ?crtl->stack_alignment_estimated = align;
> + ? ?{
> + ? ? ?unsigned int min_align = MINIMUM_ALIGNMENT (NULL, mode, align);
> + ? ? ?if (crtl->stack_alignment_estimated < min_align)
> + ? ? ? crtl->stack_alignment_estimated = min_align;
> + ? ?}
>
> ? if (generating_concat_p
> ? ? ? && (GET_MODE_CLASS (mode) == MODE_COMPLEX_FLOAT
> --- gcc/function.c.jj ? 2009-07-09 14:25:47.000000000 +0200
> +++ gcc/function.c ? ? ?2009-07-10 12:12:22.000000000 +0200
> @@ -3138,8 +3138,12 @@ assign_parms (tree fndecl)
> ? ? ? ? {
> ? ? ? ? ? unsigned int align = FUNCTION_ARG_BOUNDARY (data.promoted_mode,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?data.passed_type);
> + ? ? ? ? align = MINIMUM_ALIGNMENT (data.passed_type, data.promoted_mode,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?align);
> ? ? ? ? ?if (TYPE_ALIGN (data.nominal_type) > align)
> - ? ? ? ? ? align = TYPE_ALIGN (data.passed_type);
> + ? ? ? ? ? align = MINIMUM_ALIGNMENT (data.nominal_type,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?TYPE_MODE (data.nominal_type),
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?TYPE_ALIGN (data.nominal_type));
> ? ? ? ? ?if (crtl->stack_alignment_estimated < align)
> ? ? ? ? ? ?{
> ? ? ? ? ? ? ?gcc_assert (!crtl->stack_realign_processed);
>
> ? ? ? ?Jakub
>