* Re: [PATCH 2/2][v4] Drop excess size used for run time allocated stack variables.
@ 2016-08-24 2:02 David Edelsohn
2016-08-24 11:42 ` Andreas Krebbel
0 siblings, 1 reply; 6+ messages in thread
From: David Edelsohn @ 2016-08-24 2:02 UTC (permalink / raw)
To: Andreas Krebbel, vogt, Jeffrey Law; +Cc: GCC Patches
This patch broke bootstrap on AIX. Stage1 GCC is miscompiled.
Please revert this patch.
- David
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2][v4] Drop excess size used for run time allocated stack variables.
2016-08-24 2:02 [PATCH 2/2][v4] Drop excess size used for run time allocated stack variables David Edelsohn
@ 2016-08-24 11:42 ` Andreas Krebbel
2016-08-24 12:57 ` David Edelsohn
0 siblings, 1 reply; 6+ messages in thread
From: Andreas Krebbel @ 2016-08-24 11:42 UTC (permalink / raw)
To: David Edelsohn, vogt, Jeffrey Law; +Cc: GCC Patches
On 08/24/2016 04:02 AM, David Edelsohn wrote:
> This patch broke bootstrap on AIX. Stage1 GCC is miscompiled.
>
> Please revert this patch.
Done. Sorry for the breakage.
-Andreas-
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2][v4] Drop excess size used for run time allocated stack variables.
2016-08-24 11:42 ` Andreas Krebbel
@ 2016-08-24 12:57 ` David Edelsohn
0 siblings, 0 replies; 6+ messages in thread
From: David Edelsohn @ 2016-08-24 12:57 UTC (permalink / raw)
To: Andreas Krebbel; +Cc: vogt, Jeffrey Law, GCC Patches
On Wed, Aug 24, 2016 at 7:42 AM, Andreas Krebbel
<krebbel@linux.vnet.ibm.com> wrote:
> On 08/24/2016 04:02 AM, David Edelsohn wrote:
>> This patch broke bootstrap on AIX. Stage1 GCC is miscompiled.
>>
>> Please revert this patch.
>
> Done. Sorry for the breakage.
Is the alignment assumption safe irrespective of ABI?
Thanks, David
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2][v4] Drop excess size used for run time allocated stack variables.
2016-07-26 15:53 ` [PATCH 2/2][v4] " Dominik Vogt
2016-08-18 16:20 ` Jeff Law
@ 2016-08-23 9:23 ` Andreas Krebbel
1 sibling, 0 replies; 6+ messages in thread
From: Andreas Krebbel @ 2016-08-23 9:23 UTC (permalink / raw)
To: Dominik Vogt; +Cc: gcc-patches
> gcc/ChangeLog
>
> * explow.c (get_dynamic_stack_size): Take known alignment of stack
> pointer + STACK_DYNAMIC_OFFSET into account when calculating the size
> needed.
> Correct a typo in a comment.
Applied. Thanks!
-Andreas-
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2][v4] Drop excess size used for run time allocated stack variables.
2016-07-26 15:53 ` [PATCH 2/2][v4] " Dominik Vogt
@ 2016-08-18 16:20 ` Jeff Law
2016-08-23 9:23 ` Andreas Krebbel
1 sibling, 0 replies; 6+ messages in thread
From: Jeff Law @ 2016-08-18 16:20 UTC (permalink / raw)
To: vogt, gcc-patches, Andreas Krebbel
On 07/26/2016 09:53 AM, Dominik Vogt wrote:
> Finally a patch that works and is simple. Bootstrapped and
> regression tested on s390, s390x biarch and x86_64. The new patch
> exploits the known alignment of (stack pointer +
> STACK_DYNAMIC_OFFSET) as described earlier (see below). I think
> that is the right way to get rid of the extra allocation. It
> took a long time to understand the problem.
>
> As the patch triggers a bug in the fortran compiler, the
> der_type.f90 test case may fail on some targets if this patch is
> used without the fortran fix that I've posted in another thread.
>
> (The patch also contains a fix for a typo in a comment in the
> patched function.)
>
> See ChangeLog for a full description of the new patch.
>
> Since the patch is all new, we're not going to commit it without a
> new OK.
I like this one much better :-)
OK.
Thanks for your patience,
JEff
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2][v4] Drop excess size used for run time allocated stack variables.
2016-07-22 12:02 ` Dominik Vogt
@ 2016-07-26 15:53 ` Dominik Vogt
2016-08-18 16:20 ` Jeff Law
2016-08-23 9:23 ` Andreas Krebbel
0 siblings, 2 replies; 6+ messages in thread
From: Dominik Vogt @ 2016-07-26 15:53 UTC (permalink / raw)
To: Jeff Law, gcc-patches, Andreas Krebbel
[-- Attachment #1: Type: text/plain, Size: 2611 bytes --]
Finally a patch that works and is simple. Bootstrapped and
regression tested on s390, s390x biarch and x86_64. The new patch
exploits the known alignment of (stack pointer +
STACK_DYNAMIC_OFFSET) as described earlier (see below). I think
that is the right way to get rid of the extra allocation. It
took a long time to understand the problem.
As the patch triggers a bug in the fortran compiler, the
der_type.f90 test case may fail on some targets if this patch is
used without the fortran fix that I've posted in another thread.
(The patch also contains a fix for a typo in a comment in the
patched function.)
See ChangeLog for a full description of the new patch.
Since the patch is all new, we're not going to commit it without a
new OK.
> Actually I was goind to abandon the patch in its current state.
> :-) We talked about it internally and concluded that the problem
> is really this:
>
> * get_dynamic_stack_size is passed a SIZE of a data block (which
> is allocated elsewhere), the SIZE_ALIGN of the SIZE (i.e. the
> alignment of the underlying memory units (e.g. 32 bytes split
> into 4 times 8 bytes = 64 bit alignment) and the
> REQUIRED_ALIGN of the data portion of the allocated memory.
> * Assuming the function is called with SIZE = 2, SIZE_ALIGN = 8
> and REQUIRED_ALIGN = 64 it first adds 7 bytes to SIZE -> 9.
> This is what is needed to have two bytes 8-byte-aligned at some
> memory location without any known alignment.
> * Finally round_push is called to round up SIZE to a multiple of
> the stack slot size.
>
> The key to understanding this is that the function assumes that
> STACK_DYNMAIC_OFFSET is completely unknown at the time its called
> and therefore it does not make assumptions about the alignment of
> STACKPOINTER + STACK_DYNMAIC_OFFSET. The latest patch simply
> hard-codes that SP + SDO is supposed to be aligned to at least
> stack slot size (and does that in a very complicated way). Since
> there is no guarantee that this is the case on all targets, the
> patch is broken. It may miscalculate a SIZE that is too small in
> some cases.
>
> However, on many targets there is some guarantee about the
> alignment of SP + SDO even if the actual value of SDO is unknown.
> On s390x it's always 8-byte-aligned (stack slot size). So the
> right fix should be to add knowledge about the target's guaranteed
> alignment of SP + SDO to the function. I'm right now testing a
> much simpler patch that uses
> REGNO_POINTER_ALIGN(VIRTUAL_STACK_DYNAMIC_REGNUM) as the
> alignment.
Ciao
Dominik ^_^ ^_^
--
Dominik Vogt
IBM Germany
[-- Attachment #2: 0001-v4-ChangeLog --]
[-- Type: text/plain, Size: 193 bytes --]
gcc/ChangeLog
* explow.c (get_dynamic_stack_size): Take known alignment of stack
pointer + STACK_DYNAMIC_OFFSET into account when calculating the size
needed.
Correct a typo in a comment.
[-- Attachment #3: 0001-v4-Reduce-size-allocated-for-run-time-allocated-stack-v.patch --]
[-- Type: text/plain, Size: 2398 bytes --]
From 7d7a6b7fdb189759eb11b96c93ee4adfa8608a97 Mon Sep 17 00:00:00 2001
From: Dominik Vogt <vogt@linux.vnet.ibm.com>
Date: Fri, 29 Apr 2016 08:36:59 +0100
Subject: [PATCH] Reduce size allocated for run time allocated stack
variables.
The present calculation sometimes led to more stack memory being used than
necessary with alloca. First, (STACK_BOUNDARY -1) would be added to the
allocated size:
size = plus_constant (Pmode, size, extra);
size = force_operand (size, NULL_RTX);
Then round_push was called and added another (STACK_BOUNDARY - 1) before
rounding down to a multiple of STACK_BOUNDARY. On s390x this resulted in
adding 14 before rounding down for "x" in the test case pr36728-1.c.
The problem was that get_dynamic_stack_size did not take into account that the
target might guarantee some alignment of (stack_pointer + STACK_DYNAMIC_OFFSET)
even if the value of STACK_DYNAMIC_OFFSET is not known yet.
---
gcc/explow.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/gcc/explow.c b/gcc/explow.c
index a345690..f97a214 100644
--- a/gcc/explow.c
+++ b/gcc/explow.c
@@ -1224,9 +1224,15 @@ get_dynamic_stack_size (rtx *psize, unsigned size_align,
example), so we must preventively align the value. We leave space
in SIZE for the hole that might result from the alignment operation. */
- extra = (required_align - BITS_PER_UNIT) / BITS_PER_UNIT;
- size = plus_constant (Pmode, size, extra);
- size = force_operand (size, NULL_RTX);
+ unsigned known_align = REGNO_POINTER_ALIGN (VIRTUAL_STACK_DYNAMIC_REGNUM);
+ if (known_align == 0)
+ known_align = BITS_PER_UNIT;
+ if (required_align > known_align)
+ {
+ extra = (required_align - known_align) / BITS_PER_UNIT;
+ size = plus_constant (Pmode, size, extra);
+ size = force_operand (size, NULL_RTX);
+ }
if (flag_stack_usage_info && pstack_usage_size)
*pstack_usage_size += extra;
@@ -1235,7 +1241,7 @@ get_dynamic_stack_size (rtx *psize, unsigned size_align,
size_align = BITS_PER_UNIT;
/* Round the size to a multiple of the required stack alignment.
- Since the stack if presumed to be rounded before this allocation,
+ Since the stack is presumed to be rounded before this allocation,
this will maintain the required alignment.
If the stack grows downward, we could save an insn by subtracting
--
2.3.0
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-08-24 12:57 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-24 2:02 [PATCH 2/2][v4] Drop excess size used for run time allocated stack variables David Edelsohn
2016-08-24 11:42 ` Andreas Krebbel
2016-08-24 12:57 ` David Edelsohn
-- strict thread matches above, loose matches on Subject: below --
2016-04-29 22:13 [PATCH] " Dominik Vogt
2016-05-02 15:10 ` Jeff Law
2016-05-03 14:18 ` Dominik Vogt
2016-05-25 14:02 ` [PATCH 1/2][v3] " Dominik Vogt
2016-05-25 14:31 ` [PATCH 2/2][v3] " Dominik Vogt
2016-06-23 4:24 ` Jeff Law
2016-06-23 9:57 ` Dominik Vogt
2016-07-21 20:07 ` Jeff Law
2016-07-22 12:02 ` Dominik Vogt
2016-07-26 15:53 ` [PATCH 2/2][v4] " Dominik Vogt
2016-08-18 16:20 ` Jeff Law
2016-08-23 9:23 ` Andreas Krebbel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).