This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: PATCH: PR middle-end/36253: Caller-save stack slot may not have proper alignment
On Sun, May 25, 2008 at 06:17:02PM +0200, Richard Guenther wrote:
> On Sat, May 17, 2008 at 10:20 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> > On Sat, May 17, 2008 at 08:10:54AM -0700, H.J. Lu wrote:
> >> setup_save_areas calls assign_stack_local to allocate a stack slot for
> >> a hard register in the widest mode. assign_stack_local has
> >>
> >> if (align == 0)
> >> {
> >> tree type;
> >>
> >> if (mode == BLKmode)
> >> alignment = BIGGEST_ALIGNMENT;
> >> else
> >> alignment = GET_MODE_ALIGNMENT (mode);
> >>
> >> /* Allow the target to (possibly) increase the alignment of this
> >> stack slot. */
> >> type = lang_hooks.types.type_for_mode (mode, 0);
> >> if (type)
> >> alignment = LOCAL_ALIGNMENT (type, alignment);
> >>
> >> alignment /= BITS_PER_UNIT;
> >> }
> >>
> >> For x87, the widest mode is XF, which is 12byte aligned at 4 byte.
> >> So a 12byte stack slot aligned at 4 byte is allocated for an x87
> >> register. For x87, DF is 8 byte aligned at 8 byte. If it turns out
> >> later that we need to save/restore DF instead XF, we load/store the DF
> >> register at 4 byte instead of 8 byte. It won't give us the best x87
> >> performance.
> >>
> >> This patch fixes this problem by passing NULL type and mode to
> >> LOCAL_ALIGNMENT so that the backend can provide the largest alignment
> >> for a caller-save stack slot.
> >>
> >> This patch moves the duplicated LOCAL_ALIGNMENT definitions in
> >> cfgexpand.c and function.c to defaults.h. It also removed
> >> the duplicated stack slot alignment code.
> >>
> >> Is this a right approach?
>
> First of all I don't like the overloading of LOCAL_ALIGNMENT for the
> special case of
> allocating caller save stack slots. But thinking about it anything
> else wouldn't make
> it simpler (but maybe easier to understand).
>
> Second, changes like this:
>
> -#define LOCAL_ALIGNMENT(TYPE,ALIGN) ((ALIGN) > 128 ? (ALIGN) : 128)
> +#define LOCAL_ALIGNMENT(TYPE,MODE,ALIGN) \
> + ((!(TYPE) || (ALIGN) > 128) ? (ALIGN) : 128)
>
> do change the alignment of stack slots to less alignment. So I'd like
> all the affected
> target maintainers to have a look or you to make the change a no-op
> for all affected
> targets instead.
When LOCAL_ALIGNMENT is used on stack slot, type isn't NULL:
if (type)
alignment = LOCAL_ALIGNMENT (type, alignment);
That is if type is NULL, alignment is unchanged. My patch shouldn't
reduce alignment on stack slots.
H.J.