On 05/20/2016 01:11 AM, Jeff Law wrote:
Let's start with clean up of dead code:
/* We will need to ensure that the address we return is aligned to
REQUIRED_ALIGN. If STACK_DYNAMIC_OFFSET is defined, we don't
always know its final value at this point in the compilation (it
might depend on the size of the outgoing parameter lists, for
example), so we must align the value to be returned in that case.
(Note that STACK_DYNAMIC_OFFSET will have a default nonzero value if
STACK_POINTER_OFFSET or ACCUMULATE_OUTGOING_ARGS are defined).
We must also do an alignment operation on the returned value if
the stack pointer alignment is less strict than REQUIRED_ALIGN.
If we have to align, we must leave space in SIZE for the hole
that might result from the alignment operation. */
must_align = (crtl->preferred_stack_boundary < required_align);
if (must_align)
{
if (required_align > PREFERRED_STACK_BOUNDARY)
extra_align = PREFERRED_STACK_BOUNDARY;
else if (required_align > STACK_BOUNDARY)
extra_align = STACK_BOUNDARY;
else
extra_align = BITS_PER_UNIT;
}
/* ??? STACK_POINTER_OFFSET is always defined now. */
#if defined (STACK_DYNAMIC_OFFSET) || defined (STACK_POINTER_OFFSET)
must_align = true;
extra_align = BITS_PER_UNIT;
#endif
If we look at defaults.h, it always defines STACK_POINTER_OFFSET. So
all the code above I think collapses to:
must_align = true;
extra_align = BITS_PER_UNIT
(Cc'ing rth because portions of this seem to be his, r165240).
I kind of want to approach this from a different angle; let's look at
extra_align. The way this is used subsequently makes it appear to be
misnamed. It looks like it should hold the stack alignment we can assume:
unsigned extra = (required_align - extra_align) / BITS_PER_UNIT;
size = plus_constant (Pmode, size, extra);
[...]
if (extra && size_align > extra_align)
size_align = extra_align;
(where size_align is the known alignment of the size of the block to be
allocated). If I'm reading this right, then the first part of the
cleanup ought to be to get the naming right.
So why BITS_PER_UNIT? Shouldn't it at least be STACK_BOUNDARY? Let's
look at the previous block a little more closely.
> must_align = (crtl->preferred_stack_boundary < required_align);
[ crtl->preferred_stack_boundary is initialized to STACK_BOUNDARY in
cfgexpand and only ever increased ]
> if (must_align)
> {
[ if must_align, then required_align > crtl->p_s_b >= STACK_BOUNDARY ]
> if (required_align > PREFERRED_STACK_BOUNDARY)
> extra_align = PREFERRED_STACK_BOUNDARY;
[ so far so good ]
> else if (required_align > STACK_BOUNDARY)
> extra_align = STACK_BOUNDARY;
[ always true, right? ]
> else
> extra_align = BITS_PER_UNIT;
> }
[ dead code, right? ]
So we're left with the question of why extra_align is set to
BITS_PER_UNIT for STACK_DYNAMIC_OFFSET, and I can't really see a reason
to do that either. AFAIK the minimum alignment of the stack is always
STACK_BOUNDARY, and it's possible we could do better.
As far as I can tell, no definition of STACK_DYNAMIC_OFFSET differs
substantially from the default definition in function.c. Why couldn't we
round up the outgoing_args_size to the preferred stack boundary (or a
new value to keep track of the required alignment for dynamic
allocations) before instantiating dynamic_offset? We then wouldn't have
to add extra alignment for it here.
This rounding seems to happen anyway in port's frame calculations, e.g.
here in i386:
if (ACCUMULATE_OUTGOING_ARGS
&& (!crtl->is_leaf || cfun->calls_alloca
|| ix86_current_function_calls_tls_descriptor))
{
offset += crtl->outgoing_args_size;
frame->outgoing_arguments_size = crtl->outgoing_args_size;
}
else
frame->outgoing_arguments_size = 0;
/* Align stack boundary. Only needed if we're calling another function
or using alloca. */
if (!crtl->is_leaf || cfun->calls_alloca
|| ix86_current_function_calls_tls_descriptor)
offset = ROUND_UP (offset, preferred_alignment);
or here in rs6000:
info->parm_size = RS6000_ALIGN (crtl->outgoing_args_size,
TARGET_ALTIVEC ? 16 : 8);
which could probably use the default definition of STACK_DYNAMIC_OFFSET
instead of this, if the outgoing_args_size was rounded appropriately:
#define STACK_DYNAMIC_OFFSET(FUNDECL) \
(RS6000_ALIGN (crtl->outgoing_args_size, \
(TARGET_ALTIVEC || TARGET_VSX) ? 16 : 8) \
+ (STACK_POINTER_OFFSET))
Bernd
