On 05/03/2016 08:17 AM, Dominik Vogt wrote:
Version two of the patch including a test case.
On Mon, May 02, 2016 at 09:10:25AM -0600, Jeff Law wrote:
On 04/29/2016 04:12 PM, Dominik Vogt wrote:
The attached patch removes excess stack space allocation with
alloca in some situations. Plese check the commit message in the
patch for details.
However, I would strongly recommend some tests, even if they are
target specific. You can always copy pr36728-1 into the s390x
directory and look at size of the generated stack. Simliarly for
pr50938 for x86.
However, x86 uses the "else" branch in round_push, i.e. it uses
"virtual_preferred_stack_boundary_rtx" to calculate the number of
bytes to add for stack alignment. That value is unknown at the
time round_push is called, so the test case fails on such targets,
and I've no idea how to fix this properly.
I'm not going to be able to complete a review of this today. As I dig
into the history of this code I came across pr34548, pr47353, & pr46894
(and their associated discussions on the lists) that we'll need to
verify we don't break. This is a bit of a mess and I think the code
needs some TLC before we start hacking it up further.
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
And the only other assignment to must_align assigns it the value "true".
There are two conditionals on must_align that looks like
if (must_align)
{
CODE;
}
We should remove the conditional and pull CODE out an indentation level.
And remove all remnants of must_align.
I don't think that changes your patch in any way. Hopefully it makes
the whole function somewhat easier to grok.
Thoughts?
jeff
