* 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
* [PATCH] Drop excess size used for run time allocated stack variables. @ 2016-04-29 22:13 Dominik Vogt 2016-05-02 15:10 ` Jeff Law 0 siblings, 1 reply; 6+ messages in thread From: Dominik Vogt @ 2016-04-29 22:13 UTC (permalink / raw) To: gcc-patches; +Cc: Andreas Krebbel [-- Attachment #1: Type: text/plain, Size: 202 bytes --] The attached patch removes excess stack space allocation with alloca in some situations. Plese check the commit message in the patch for details. Ciao Dominik ^_^ ^_^ -- Dominik Vogt IBM Germany [-- Attachment #2: 0001-ChangeLog --] [-- Type: text/plain, Size: 132 bytes --] gcc/ChangeLog * explow.c (round_push): Use know adjustment. (allocate_dynamic_stack_space): Pass known adjustment to round_push. [-- Attachment #3: 0001-Drop-excess-size-used-for-run-time-allocated-stack-v.patch --] [-- Type: text/x-diff, Size: 4520 bytes --] From 9ea451aef0f1f2fb0a36a7b718f910cfe285541d 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] Drop excess size used 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. round_push() now takes an argument to inform it about what has already been added to size. --- gcc/explow.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/gcc/explow.c b/gcc/explow.c index e0ce201..a039295 100644 --- a/gcc/explow.c +++ b/gcc/explow.c @@ -949,24 +949,30 @@ anti_adjust_stack (rtx adjust) } /* Round the size of a block to be pushed up to the boundary required - by this machine. SIZE is the desired size, which need not be constant. */ + by this machine. SIZE is the desired size, which need not be constant. + ALREADY_ADDED is the number of units that have already been added to SIZE for + other alignment reasons. +*/ static rtx -round_push (rtx size) +round_push (rtx size, int already_added) { - rtx align_rtx, alignm1_rtx; + rtx align_rtx, add_rtx; if (!SUPPORTS_STACK_ALIGNMENT || crtl->preferred_stack_boundary == MAX_SUPPORTED_STACK_ALIGNMENT) { int align = crtl->preferred_stack_boundary / BITS_PER_UNIT; + int add; if (align == 1) return size; + add = (align > already_added) ? align - already_added - 1 : 0; + if (CONST_INT_P (size)) { - HOST_WIDE_INT new_size = (INTVAL (size) + align - 1) / align * align; + HOST_WIDE_INT new_size = (INTVAL (size) + add) / align * align; if (INTVAL (size) != new_size) size = GEN_INT (new_size); @@ -974,7 +980,7 @@ round_push (rtx size) } align_rtx = GEN_INT (align); - alignm1_rtx = GEN_INT (align - 1); + add_rtx = (add > 0) ? GEN_INT (add) : const0_rtx; } else { @@ -983,15 +989,15 @@ round_push (rtx size) substituted by the right value in vregs pass and optimized during combine. */ align_rtx = virtual_preferred_stack_boundary_rtx; - alignm1_rtx = force_operand (plus_constant (Pmode, align_rtx, -1), - NULL_RTX); + add_rtx = force_operand (plus_constant (Pmode, align_rtx, -1), NULL_RTX); } /* CEIL_DIV_EXPR needs to worry about the addition overflowing, but we know it can't. So add ourselves and then do TRUNC_DIV_EXPR. */ - size = expand_binop (Pmode, add_optab, size, alignm1_rtx, - NULL_RTX, 1, OPTAB_LIB_WIDEN); + if (add_rtx != const0_rtx) + size = expand_binop (Pmode, add_optab, size, add_rtx, + NULL_RTX, 1, OPTAB_LIB_WIDEN); size = expand_divmod (0, TRUNC_DIV_EXPR, Pmode, size, align_rtx, NULL_RTX, 1); size = expand_mult (Pmode, size, align_rtx, NULL_RTX, 1); @@ -1175,6 +1181,7 @@ allocate_dynamic_stack_space (rtx size, unsigned size_align, rtx_code_label *final_label; rtx final_target, target; unsigned extra_align = 0; + unsigned extra = 0; bool must_align; /* If we're asking for zero bytes, it doesn't matter what we point @@ -1275,9 +1282,9 @@ allocate_dynamic_stack_space (rtx size, unsigned size_align, extra_align = BITS_PER_UNIT; #endif - if (must_align) + if (must_align && required_align > extra_align) { - unsigned extra = (required_align - extra_align) / BITS_PER_UNIT; + extra = (required_align - extra_align) / BITS_PER_UNIT; size = plus_constant (Pmode, size, extra); size = force_operand (size, NULL_RTX); @@ -1285,7 +1292,7 @@ allocate_dynamic_stack_space (rtx size, unsigned size_align, if (flag_stack_usage_info) stack_usage_size += extra; - if (extra && size_align > extra_align) + if (size_align > extra_align) size_align = extra_align; } @@ -1304,7 +1311,7 @@ allocate_dynamic_stack_space (rtx size, unsigned size_align, momentarily mis-aligning the stack. */ if (size_align % MAX_SUPPORTED_STACK_ALIGNMENT != 0) { - size = round_push (size); + size = round_push (size, extra); if (flag_stack_usage_info) { -- 2.3.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Drop excess size used for run time allocated stack variables. 2016-04-29 22:13 [PATCH] " Dominik Vogt @ 2016-05-02 15:10 ` Jeff Law 2016-05-03 14:18 ` Dominik Vogt 0 siblings, 1 reply; 6+ messages in thread From: Jeff Law @ 2016-05-02 15:10 UTC (permalink / raw) To: vogt, gcc-patches, Andreas Krebbel 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. > > Ciao > > Dominik ^_^ ^_^ > > -- Dominik Vogt IBM Germany > > > 0001-ChangeLog > > > gcc/ChangeLog > > * explow.c (round_push): Use know adjustment. > (allocate_dynamic_stack_space): Pass known adjustment to round_push. If I understand the state of this patch correctly, you're working on another iteration, so I'm not going to dig into this version. 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. jeff ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Drop excess size used for run time allocated stack variables. 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 0 siblings, 1 reply; 6+ messages in thread From: Dominik Vogt @ 2016-05-03 14:18 UTC (permalink / raw) To: Jeff Law; +Cc: gcc-patches, Andreas Krebbel [-- Attachment #1: Type: text/plain, Size: 889 bytes --] 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. Ciao Dominik ^_^ ^_^ -- Dominik Vogt IBM Germany [-- Attachment #2: 0001-v2-ChangeLog --] [-- Type: text/plain, Size: 188 bytes --] gcc/ChangeLog * explow.c (round_push): Use know adjustment. (allocate_dynamic_stack_space): Pass known adjustment to round_push. gcc/testsuite/ChangeLog * gcc.dg/pr50938.c: New test. [-- Attachment #3: 0001-v2-Drop-excess-size-used-for-run-time-allocated-stack-v.patch --] [-- Type: text/x-diff, Size: 5950 bytes --] From f4d252eb2c860450b3738591fca36f568c958232 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] Drop excess size used 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. round_push() now takes an argument to inform it about what has already been added to size. --- gcc/explow.c | 33 ++++++++++++++++----------- gcc/testsuite/gcc.dg/pr50938.c | 52 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 13 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/pr50938.c diff --git a/gcc/explow.c b/gcc/explow.c index e0ce201..1858597 100644 --- a/gcc/explow.c +++ b/gcc/explow.c @@ -949,24 +949,30 @@ anti_adjust_stack (rtx adjust) } /* Round the size of a block to be pushed up to the boundary required - by this machine. SIZE is the desired size, which need not be constant. */ + by this machine. SIZE is the desired size, which need not be constant. + ALREADY_ADDED is the number of units that have already been added to SIZE + for other alignment reasons. +*/ static rtx -round_push (rtx size) +round_push (rtx size, int already_added) { - rtx align_rtx, alignm1_rtx; + rtx align_rtx, add_rtx; if (!SUPPORTS_STACK_ALIGNMENT || crtl->preferred_stack_boundary == MAX_SUPPORTED_STACK_ALIGNMENT) { int align = crtl->preferred_stack_boundary / BITS_PER_UNIT; + int add; if (align == 1) return size; + add = (align > already_added) ? align - already_added - 1 : 0; + if (CONST_INT_P (size)) { - HOST_WIDE_INT new_size = (INTVAL (size) + align - 1) / align * align; + HOST_WIDE_INT new_size = (INTVAL (size) + add) / align * align; if (INTVAL (size) != new_size) size = GEN_INT (new_size); @@ -974,7 +980,7 @@ round_push (rtx size) } align_rtx = GEN_INT (align); - alignm1_rtx = GEN_INT (align - 1); + add_rtx = (add > 0) ? GEN_INT (add) : const0_rtx; } else { @@ -983,15 +989,15 @@ round_push (rtx size) substituted by the right value in vregs pass and optimized during combine. */ align_rtx = virtual_preferred_stack_boundary_rtx; - alignm1_rtx = force_operand (plus_constant (Pmode, align_rtx, -1), - NULL_RTX); + add_rtx = force_operand (plus_constant (Pmode, align_rtx, -1), NULL_RTX); } /* CEIL_DIV_EXPR needs to worry about the addition overflowing, but we know it can't. So add ourselves and then do TRUNC_DIV_EXPR. */ - size = expand_binop (Pmode, add_optab, size, alignm1_rtx, - NULL_RTX, 1, OPTAB_LIB_WIDEN); + if (add_rtx != const0_rtx) + size = expand_binop (Pmode, add_optab, size, add_rtx, + NULL_RTX, 1, OPTAB_LIB_WIDEN); size = expand_divmod (0, TRUNC_DIV_EXPR, Pmode, size, align_rtx, NULL_RTX, 1); size = expand_mult (Pmode, size, align_rtx, NULL_RTX, 1); @@ -1175,6 +1181,7 @@ allocate_dynamic_stack_space (rtx size, unsigned size_align, rtx_code_label *final_label; rtx final_target, target; unsigned extra_align = 0; + unsigned extra = 0; bool must_align; /* If we're asking for zero bytes, it doesn't matter what we point @@ -1275,9 +1282,9 @@ allocate_dynamic_stack_space (rtx size, unsigned size_align, extra_align = BITS_PER_UNIT; #endif - if (must_align) + if (must_align && required_align > extra_align) { - unsigned extra = (required_align - extra_align) / BITS_PER_UNIT; + extra = (required_align - extra_align) / BITS_PER_UNIT; size = plus_constant (Pmode, size, extra); size = force_operand (size, NULL_RTX); @@ -1285,7 +1292,7 @@ allocate_dynamic_stack_space (rtx size, unsigned size_align, if (flag_stack_usage_info) stack_usage_size += extra; - if (extra && size_align > extra_align) + if (size_align > extra_align) size_align = extra_align; } @@ -1304,7 +1311,7 @@ allocate_dynamic_stack_space (rtx size, unsigned size_align, momentarily mis-aligning the stack. */ if (size_align % MAX_SUPPORTED_STACK_ALIGNMENT != 0) { - size = round_push (size); + size = round_push (size, extra); if (flag_stack_usage_info) { diff --git a/gcc/testsuite/gcc.dg/pr50938.c b/gcc/testsuite/gcc.dg/pr50938.c new file mode 100644 index 0000000..14b7540 --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr50938.c @@ -0,0 +1,52 @@ +/* PR/50938: Check that alloca () reserves the correct amount of stack space. + */ + +/* { dg-do run } */ +/* { dg-options "-O0" } */ +/* { dg-require-effective-target alloca } */ + +__attribute__ ((noinline)) +static void +dummy (void) +{ +} + +__attribute__ ((noinline)) +static char * +get_frame_addr (void *p) +{ + void *faddr = __builtin_frame_address (0); + /* Call a function to make sure that a stack frame exists. */ + dummy (); + return faddr; +} + +__attribute__ ((noinline)) +static void * +stack_var (void) +{ + /* 1024 bytes on the stack. */ + char s[1024]; + /* One stack slot. */ + void *p = (void *)s; + /* Keep stack memory alive by passing it to the function. */ + return get_frame_addr (p); +} + +__attribute__ ((noinline)) +static void * +alloca_var (void) +{ + /* 1024 bytes on the stack plus one stack slot for a. */ + void *a = __builtin_alloca (1024); + return get_frame_addr (a); +} + +int main (void) +{ + if (stack_var () != alloca_var ()) + /* The functions used a different amount of stack space. */ + __builtin_abort (); + + return 0; +} -- 2.3.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2][v3] Drop excess size used for run time allocated stack variables. 2016-05-03 14:18 ` Dominik Vogt @ 2016-05-25 14:02 ` Dominik Vogt 2016-05-25 14:31 ` [PATCH 2/2][v3] " Dominik Vogt 0 siblings, 1 reply; 6+ messages in thread From: Dominik Vogt @ 2016-05-25 14:02 UTC (permalink / raw) To: Jeff Law, gcc-patches, Andreas Krebbel [-- Attachment #1: Type: text/plain, Size: 1332 bytes --] On Tue, May 03, 2016 at 03:17:53PM +0100, 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. Third version of the patch with the suggested cleanup in the first patch and the functional stuff in the second one. The first patch is based on Jeff's draft with the change suggested by Eric and more cleanup added by me. Tested and bootstrapped on s390x biarch (but did not look for performance regressions as the change should be a no-op). Ciao Dominik ^_^ ^_^ -- Dominik Vogt IBM Germany [-- Attachment #2: 0001-ChangeLog --] [-- Type: text/plain, Size: 152 bytes --] gcc/ChangeLog0 * explow.c (allocate_dynamic_stack_space): Simplify knowing that MUST_ALIGN was always true and extra_align ist always BITS_PER_UNIT. [-- Attachment #3: 0001-Minor-cleanup-to-allocate_dynamic_stack_space.patch --] [-- Type: text/x-diff, Size: 5503 bytes --] From 01264dac2f62c16a2c7e684a674aa9e9bc27b434 Mon Sep 17 00:00:00 2001 From: Dominik Vogt <vogt@linux.vnet.ibm.com> Date: Wed, 25 May 2016 10:23:57 +0100 Subject: [PATCH 1/2] Minor cleanup to allocate_dynamic_stack_space. --- gcc/explow.c | 96 +++++++++++++++++++----------------------------------------- 1 file changed, 30 insertions(+), 66 deletions(-) diff --git a/gcc/explow.c b/gcc/explow.c index e0ce201..09a0330 100644 --- a/gcc/explow.c +++ b/gcc/explow.c @@ -1174,8 +1174,7 @@ allocate_dynamic_stack_space (rtx size, unsigned size_align, HOST_WIDE_INT stack_usage_size = -1; rtx_code_label *final_label; rtx final_target, target; - unsigned extra_align = 0; - bool must_align; + unsigned extra; /* If we're asking for zero bytes, it doesn't matter what we point to since we can't dereference it. But return a reasonable @@ -1246,48 +1245,21 @@ allocate_dynamic_stack_space (rtx size, unsigned size_align, crtl->preferred_stack_boundary = PREFERRED_STACK_BOUNDARY; /* 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; - } + REQUIRED_ALIGN. At this point in the compilation, we don't always + know the final value of the STACK_DYNAMIC_OFFSET used in function.c + (it might depend on the size of the outgoing parameter lists, for + example), so we must preventively align the value. We leave space + in SIZE for the hole that might result from the alignment operation. */ - /* ??? 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 (must_align) - { - unsigned extra = (required_align - extra_align) / BITS_PER_UNIT; + extra = (required_align - BITS_PER_UNIT) / BITS_PER_UNIT; + size = plus_constant (Pmode, size, extra); + size = force_operand (size, NULL_RTX); - size = plus_constant (Pmode, size, extra); - size = force_operand (size, NULL_RTX); - - if (flag_stack_usage_info) - stack_usage_size += extra; + if (flag_stack_usage_info) + stack_usage_size += extra; - if (extra && size_align > extra_align) - size_align = extra_align; - } + if (extra && size_align > BITS_PER_UNIT) + 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, @@ -1361,13 +1333,10 @@ allocate_dynamic_stack_space (rtx size, unsigned size_align, if (MALLOC_ABI_ALIGNMENT >= required_align) ask = size; else - { - ask = expand_binop (Pmode, add_optab, size, - gen_int_mode (required_align / BITS_PER_UNIT - 1, - Pmode), - NULL_RTX, 1, OPTAB_LIB_WIDEN); - must_align = true; - } + ask = expand_binop (Pmode, add_optab, size, + gen_int_mode (required_align / BITS_PER_UNIT - 1, + Pmode), + NULL_RTX, 1, OPTAB_LIB_WIDEN); func = init_one_libfunc ("__morestack_allocate_stack_space"); @@ -1478,24 +1447,19 @@ allocate_dynamic_stack_space (rtx size, unsigned size_align, target = final_target; } - if (must_align) - { - /* CEIL_DIV_EXPR needs to worry about the addition overflowing, - but we know it can't. So add ourselves and then do - TRUNC_DIV_EXPR. */ - target = expand_binop (Pmode, add_optab, target, - gen_int_mode (required_align / BITS_PER_UNIT - 1, - Pmode), - NULL_RTX, 1, OPTAB_LIB_WIDEN); - target = expand_divmod (0, TRUNC_DIV_EXPR, Pmode, target, - gen_int_mode (required_align / BITS_PER_UNIT, - Pmode), - NULL_RTX, 1); - target = expand_mult (Pmode, target, - gen_int_mode (required_align / BITS_PER_UNIT, - Pmode), - NULL_RTX, 1); - } + /* CEIL_DIV_EXPR needs to worry about the addition overflowing, + but we know it can't. So add ourselves and then do + TRUNC_DIV_EXPR. */ + target = expand_binop (Pmode, add_optab, target, + gen_int_mode (required_align / BITS_PER_UNIT - 1, + Pmode), + NULL_RTX, 1, OPTAB_LIB_WIDEN); + target = expand_divmod (0, TRUNC_DIV_EXPR, Pmode, target, + gen_int_mode (required_align / BITS_PER_UNIT, Pmode), + NULL_RTX, 1); + target = expand_mult (Pmode, target, + gen_int_mode (required_align / BITS_PER_UNIT, Pmode), + NULL_RTX, 1); /* Now that we've committed to a return value, mark its alignment. */ mark_reg_pointer (target, required_align); -- 2.3.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2][v3] Drop excess size used for run time allocated stack variables. 2016-05-25 14:02 ` [PATCH 1/2][v3] " Dominik Vogt @ 2016-05-25 14:31 ` Dominik Vogt 2016-06-23 4:24 ` Jeff Law 0 siblings, 1 reply; 6+ messages in thread From: Dominik Vogt @ 2016-05-25 14:31 UTC (permalink / raw) To: Jeff Law, gcc-patches, Andreas Krebbel [-- Attachment #1: Type: text/plain, Size: 1519 bytes --] On Wed, May 25, 2016 at 02:30:54PM +0100, Dominik Vogt wrote: > On Tue, May 03, 2016 at 03:17:53PM +0100, 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. > > Third version of the patch with the suggested cleanup in the first > patch and the functional stuff in the second one. The first patch > is based on Jeff's draft with the change suggested by Eric and > more cleanup added by me. This is the updated funtional patch. Re-tested with limited effort, i.e. tested and bootstrapped on s390x biarch (but did not look for performance regressions compared to version 2 of the patch). Ciao Dominik ^_^ ^_^ -- Dominik Vogt IBM Germany [-- Attachment #2: 0002-v3-ChangeLog --] [-- Type: text/plain, Size: 188 bytes --] gcc/ChangeLog * explow.c (round_push): Use know adjustment. (allocate_dynamic_stack_space): Pass known adjustment to round_push. gcc/testsuite/ChangeLog * gcc.dg/pr50938.c: New test. [-- Attachment #3: 0002-v3-Drop-excess-size-used-for-run-time-allocated-stack-v.patch --] [-- Type: text/x-diff, Size: 6354 bytes --] From 4296d353e1d153b5b5ee435a44cae6117bf2fff0 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 2/2] Drop excess size used 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. round_push() now takes an argument to inform it about what has already been added to size. --- gcc/explow.c | 45 +++++++++++++++++++++--------------- gcc/testsuite/gcc.dg/pr50938.c | 52 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 18 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/pr50938.c diff --git a/gcc/explow.c b/gcc/explow.c index 09a0330..85596e2 100644 --- a/gcc/explow.c +++ b/gcc/explow.c @@ -949,24 +949,30 @@ anti_adjust_stack (rtx adjust) } /* Round the size of a block to be pushed up to the boundary required - by this machine. SIZE is the desired size, which need not be constant. */ + by this machine. SIZE is the desired size, which need not be constant. + ALREADY_ADDED is the number of units that have already been added to SIZE + for other alignment reasons. +*/ static rtx -round_push (rtx size) +round_push (rtx size, int already_added) { - rtx align_rtx, alignm1_rtx; + rtx align_rtx, add_rtx; if (!SUPPORTS_STACK_ALIGNMENT || crtl->preferred_stack_boundary == MAX_SUPPORTED_STACK_ALIGNMENT) { int align = crtl->preferred_stack_boundary / BITS_PER_UNIT; + int add; if (align == 1) return size; + add = (align > already_added) ? align - already_added - 1 : 0; + if (CONST_INT_P (size)) { - HOST_WIDE_INT new_size = (INTVAL (size) + align - 1) / align * align; + HOST_WIDE_INT new_size = (INTVAL (size) + add) / align * align; if (INTVAL (size) != new_size) size = GEN_INT (new_size); @@ -974,7 +980,7 @@ round_push (rtx size) } align_rtx = GEN_INT (align); - alignm1_rtx = GEN_INT (align - 1); + add_rtx = (add > 0) ? GEN_INT (add) : const0_rtx; } else { @@ -983,15 +989,15 @@ round_push (rtx size) substituted by the right value in vregs pass and optimized during combine. */ align_rtx = virtual_preferred_stack_boundary_rtx; - alignm1_rtx = force_operand (plus_constant (Pmode, align_rtx, -1), - NULL_RTX); + add_rtx = force_operand (plus_constant (Pmode, align_rtx, -1), NULL_RTX); } /* CEIL_DIV_EXPR needs to worry about the addition overflowing, but we know it can't. So add ourselves and then do TRUNC_DIV_EXPR. */ - size = expand_binop (Pmode, add_optab, size, alignm1_rtx, - NULL_RTX, 1, OPTAB_LIB_WIDEN); + if (add_rtx != const0_rtx) + size = expand_binop (Pmode, add_optab, size, add_rtx, + NULL_RTX, 1, OPTAB_LIB_WIDEN); size = expand_divmod (0, TRUNC_DIV_EXPR, Pmode, size, align_rtx, NULL_RTX, 1); size = expand_mult (Pmode, size, align_rtx, NULL_RTX, 1); @@ -1174,7 +1180,7 @@ allocate_dynamic_stack_space (rtx size, unsigned size_align, HOST_WIDE_INT stack_usage_size = -1; rtx_code_label *final_label; rtx final_target, target; - unsigned extra; + unsigned extra = 0; /* If we're asking for zero bytes, it doesn't matter what we point to since we can't dereference it. But return a reasonable @@ -1251,15 +1257,18 @@ allocate_dynamic_stack_space (rtx size, 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); + if (required_align > BITS_PER_UNIT) + { + extra = (required_align - BITS_PER_UNIT) / BITS_PER_UNIT; + size = plus_constant (Pmode, size, extra); + size = force_operand (size, NULL_RTX); - if (flag_stack_usage_info) - stack_usage_size += extra; + if (flag_stack_usage_info) + stack_usage_size += extra; - if (extra && size_align > BITS_PER_UNIT) - size_align = BITS_PER_UNIT; + if (size_align > BITS_PER_UNIT) + 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, @@ -1276,7 +1285,7 @@ allocate_dynamic_stack_space (rtx size, unsigned size_align, momentarily mis-aligning the stack. */ if (size_align % MAX_SUPPORTED_STACK_ALIGNMENT != 0) { - size = round_push (size); + size = round_push (size, extra); if (flag_stack_usage_info) { diff --git a/gcc/testsuite/gcc.dg/pr50938.c b/gcc/testsuite/gcc.dg/pr50938.c new file mode 100644 index 0000000..14b7540 --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr50938.c @@ -0,0 +1,52 @@ +/* PR/50938: Check that alloca () reserves the correct amount of stack space. + */ + +/* { dg-do run } */ +/* { dg-options "-O0" } */ +/* { dg-require-effective-target alloca } */ + +__attribute__ ((noinline)) +static void +dummy (void) +{ +} + +__attribute__ ((noinline)) +static char * +get_frame_addr (void *p) +{ + void *faddr = __builtin_frame_address (0); + /* Call a function to make sure that a stack frame exists. */ + dummy (); + return faddr; +} + +__attribute__ ((noinline)) +static void * +stack_var (void) +{ + /* 1024 bytes on the stack. */ + char s[1024]; + /* One stack slot. */ + void *p = (void *)s; + /* Keep stack memory alive by passing it to the function. */ + return get_frame_addr (p); +} + +__attribute__ ((noinline)) +static void * +alloca_var (void) +{ + /* 1024 bytes on the stack plus one stack slot for a. */ + void *a = __builtin_alloca (1024); + return get_frame_addr (a); +} + +int main (void) +{ + if (stack_var () != alloca_var ()) + /* The functions used a different amount of stack space. */ + __builtin_abort (); + + return 0; +} -- 2.3.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2][v3] Drop excess size used for run time allocated stack variables. 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 0 siblings, 1 reply; 6+ messages in thread From: Jeff Law @ 2016-06-23 4:24 UTC (permalink / raw) To: vogt, gcc-patches, Andreas Krebbel On 05/25/2016 07:32 AM, Dominik Vogt wrote: > On Wed, May 25, 2016 at 02:30:54PM +0100, Dominik Vogt wrote: >> > On Tue, May 03, 2016 at 03:17:53PM +0100, 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. >> > >> > Third version of the patch with the suggested cleanup in the first >> > patch and the functional stuff in the second one. The first patch >> > is based on Jeff's draft with the change suggested by Eric and >> > more cleanup added by me. > This is the updated funtional patch. Re-tested with limited > effort, i.e. tested and bootstrapped on s390x biarch (but did not > look for performance regressions compared to version 2 of the > patch). > > Ciao > > Dominik ^_^ ^_^ > > -- Dominik Vogt IBM Germany > > > 0002-v3-ChangeLog > > > gcc/ChangeLog > > * explow.c (round_push): Use know adjustment. > (allocate_dynamic_stack_space): Pass known adjustment to round_push. > gcc/testsuite/ChangeLog > > * gcc.dg/pr50938.c: New test. > > > 0002-v3-Drop-excess-size-used-for-run-time-allocated-stack-v.patch > > > From 4296d353e1d153b5b5ee435a44cae6117bf2fff0 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 2/2] Drop excess size used 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. > > round_push() now takes an argument to inform it about what has already been > added to size. > --- > gcc/explow.c | 45 +++++++++++++++++++++--------------- > gcc/testsuite/gcc.dg/pr50938.c | 52 ++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 79 insertions(+), 18 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/pr50938.c > > diff --git a/gcc/explow.c b/gcc/explow.c > index 09a0330..85596e2 100644 > --- a/gcc/explow.c > +++ b/gcc/explow.c > @@ -949,24 +949,30 @@ anti_adjust_stack (rtx adjust) > } > > /* Round the size of a block to be pushed up to the boundary required > - by this machine. SIZE is the desired size, which need not be constant. */ > + by this machine. SIZE is the desired size, which need not be constant. > + ALREADY_ADDED is the number of units that have already been added to SIZE > + for other alignment reasons. > +*/ > > static rtx > -round_push (rtx size) > +round_push (rtx size, int already_added) > { > - rtx align_rtx, alignm1_rtx; > + rtx align_rtx, add_rtx; > > if (!SUPPORTS_STACK_ALIGNMENT > || crtl->preferred_stack_boundary == MAX_SUPPORTED_STACK_ALIGNMENT) > { > int align = crtl->preferred_stack_boundary / BITS_PER_UNIT; > + int add; > > if (align == 1) > return size; > > + add = (align > already_added) ? align - already_added - 1 : 0; > + > if (CONST_INT_P (size)) > { > - HOST_WIDE_INT new_size = (INTVAL (size) + align - 1) / align * align; > + HOST_WIDE_INT new_size = (INTVAL (size) + add) / align * align; > > if (INTVAL (size) != new_size) > size = GEN_INT (new_size); So presumably the idea here is when the requested SIZE would require allocating additional space to first see if the necessary space is already available inside ALREADY_ADDED and use that rather than rounding size up to an alignment boundary. I can see how that works in the sense of avoiding allocating extra space. What I'm struggling with is how do we know the space actually allocated is going to have the right alignment? What am I missing here? jeff ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2][v3] Drop excess size used for run time allocated stack variables. 2016-06-23 4:24 ` Jeff Law @ 2016-06-23 9:57 ` Dominik Vogt 2016-07-21 20:07 ` Jeff Law 0 siblings, 1 reply; 6+ messages in thread From: Dominik Vogt @ 2016-06-23 9:57 UTC (permalink / raw) To: Jeff Law; +Cc: gcc-patches, Andreas Krebbel On Wed, Jun 22, 2016 at 10:24:02PM -0600, Jeff Law wrote: > On 05/25/2016 07:32 AM, Dominik Vogt wrote: > >On Wed, May 25, 2016 at 02:30:54PM +0100, Dominik Vogt wrote: > >>> On Tue, May 03, 2016 at 03:17:53PM +0100, 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. > >>> > >>> Third version of the patch with the suggested cleanup in the first > >>> patch and the functional stuff in the second one. The first patch > >>> is based on Jeff's draft with the change suggested by Eric and > >>> more cleanup added by me. > >This is the updated funtional patch. Re-tested with limited > >effort, i.e. tested and bootstrapped on s390x biarch (but did not > >look for performance regressions compared to version 2 of the > >patch). > > > >Ciao > > > >Dominik ^_^ ^_^ > > > >-- Dominik Vogt IBM Germany > > > > > >0002-v3-ChangeLog > > > > > >gcc/ChangeLog > > > > * explow.c (round_push): Use know adjustment. > > (allocate_dynamic_stack_space): Pass known adjustment to round_push. > >gcc/testsuite/ChangeLog > > > > * gcc.dg/pr50938.c: New test. > > > > > >0002-v3-Drop-excess-size-used-for-run-time-allocated-stack-v.patch > > > > > >From 4296d353e1d153b5b5ee435a44cae6117bf2fff0 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 2/2] Drop excess size used 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. > > > >round_push() now takes an argument to inform it about what has already been > >added to size. > >--- > > gcc/explow.c | 45 +++++++++++++++++++++--------------- > > gcc/testsuite/gcc.dg/pr50938.c | 52 ++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 79 insertions(+), 18 deletions(-) > > create mode 100644 gcc/testsuite/gcc.dg/pr50938.c > > > >diff --git a/gcc/explow.c b/gcc/explow.c > >index 09a0330..85596e2 100644 > >--- a/gcc/explow.c > >+++ b/gcc/explow.c > >@@ -949,24 +949,30 @@ anti_adjust_stack (rtx adjust) > > } > > > > /* Round the size of a block to be pushed up to the boundary required > >- by this machine. SIZE is the desired size, which need not be constant. */ > >+ by this machine. SIZE is the desired size, which need not be constant. > >+ ALREADY_ADDED is the number of units that have already been added to SIZE > >+ for other alignment reasons. > >+*/ > > > > static rtx > >-round_push (rtx size) > >+round_push (rtx size, int already_added) > > { > >- rtx align_rtx, alignm1_rtx; > >+ rtx align_rtx, add_rtx; > > > > if (!SUPPORTS_STACK_ALIGNMENT > > || crtl->preferred_stack_boundary == MAX_SUPPORTED_STACK_ALIGNMENT) > > { > > int align = crtl->preferred_stack_boundary / BITS_PER_UNIT; > >+ int add; > > > > if (align == 1) > > return size; > > > >+ add = (align > already_added) ? align - already_added - 1 : 0; > >+ > > if (CONST_INT_P (size)) > > { > >- HOST_WIDE_INT new_size = (INTVAL (size) + align - 1) / align * align; > >+ HOST_WIDE_INT new_size = (INTVAL (size) + add) / align * align; > > > > if (INTVAL (size) != new_size) > > size = GEN_INT (new_size); > So presumably the idea here is when the requested SIZE would require > allocating additional space to first see if the necessary space is > already available inside ALREADY_ADDED Yes. > and use that rather than rounding size up to an alignment boundary. Not exactly. Consider the unpatched code. At the beginning we have some amount of space to be allocated on the stack at runtime ("SSIZE"), some requested alignment for it ("SALIGN"). get_dynamic_stack_size() first calculates the space needed for run time alignment: SIZE = SSIZE + SALIGN - 1 Then it calls round_push() to add *another* chunk of memory to the allocation size to be able to align it to the required stack slot alignment ("SLOTALIGN") at run time. SIZE = SIZE + SLOTALIGN - 1 = SSIZE + (SALIGN - 1) + (SLOTALIGN - 1) Now it has added two chunks of memory but alignment is only done once. With the patch it just adds the maximum of (SALIGN - 1) and (SLOTALIGN - 1), not both. Thinking about it, the "round_push" stuff is a very complicated way of saying "add max(A, B)". I'd volunteer to clean this up more, but preferrably when the two pending patches are in. The current code is a real brain-twister. > I can see how that works in the sense of avoiding allocating extra > space. What I'm struggling with is how do we know the space > actually allocated is going to have the right alignment? It doesn't. That is what the extra space is needed for, i.e. the data is placed in the (larger) allocated space at an address with proper alignment, at run time. Ciao Dominik ^_^ ^_^ -- Dominik Vogt IBM Germany ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2][v3] Drop excess size used for run time allocated stack variables. 2016-06-23 9:57 ` Dominik Vogt @ 2016-07-21 20:07 ` Jeff Law 2016-07-22 12:02 ` Dominik Vogt 0 siblings, 1 reply; 6+ messages in thread From: Jeff Law @ 2016-07-21 20:07 UTC (permalink / raw) To: vogt, gcc-patches, Andreas Krebbel On 06/23/2016 03:57 AM, Dominik Vogt wrote: >>> 0002-v3-ChangeLog >>> >>> >>> gcc/ChangeLog >>> >>> * explow.c (round_push): Use know adjustment. >>> (allocate_dynamic_stack_space): Pass known adjustment to round_push. >>> gcc/testsuite/ChangeLog >>> >>> * gcc.dg/pr50938.c: New test. >>> >>> >>> 0002-v3-Drop-excess-size-used-for-run-time-allocated-stack-v.patch >>> >>> >> >From 4296d353e1d153b5b5ee435a44cae6117bf2fff0 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 2/2] Drop excess size used 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. >>> >>> round_push() now takes an argument to inform it about what has already been >>> added to size. >>> --- >>> gcc/explow.c | 45 +++++++++++++++++++++--------------- >>> gcc/testsuite/gcc.dg/pr50938.c | 52 ++++++++++++++++++++++++++++++++++++++++++ >>> 2 files changed, 79 insertions(+), 18 deletions(-) >>> create mode 100644 gcc/testsuite/gcc.dg/pr50938.c >>> >>> diff --git a/gcc/explow.c b/gcc/explow.c >>> index 09a0330..85596e2 100644 >>> --- a/gcc/explow.c >>> +++ b/gcc/explow.c >>> @@ -949,24 +949,30 @@ anti_adjust_stack (rtx adjust) >>> } >>> >>> /* Round the size of a block to be pushed up to the boundary required >>> - by this machine. SIZE is the desired size, which need not be constant. */ >>> + by this machine. SIZE is the desired size, which need not be constant. >>> + ALREADY_ADDED is the number of units that have already been added to SIZE >>> + for other alignment reasons. >>> +*/ >>> >>> static rtx >>> -round_push (rtx size) >>> +round_push (rtx size, int already_added) >>> { >>> - rtx align_rtx, alignm1_rtx; >>> + rtx align_rtx, add_rtx; >>> >>> if (!SUPPORTS_STACK_ALIGNMENT >>> || crtl->preferred_stack_boundary == MAX_SUPPORTED_STACK_ALIGNMENT) >>> { >>> int align = crtl->preferred_stack_boundary / BITS_PER_UNIT; >>> + int add; >>> >>> if (align == 1) >>> return size; >>> >>> + add = (align > already_added) ? align - already_added - 1 : 0; >>> + >>> if (CONST_INT_P (size)) >>> { >>> - HOST_WIDE_INT new_size = (INTVAL (size) + align - 1) / align * align; >>> + HOST_WIDE_INT new_size = (INTVAL (size) + add) / align * align; >>> >>> if (INTVAL (size) != new_size) >>> size = GEN_INT (new_size); >> So presumably the idea here is when the requested SIZE would require >> allocating additional space to first see if the necessary space is >> already available inside ALREADY_ADDED > > Yes. > >> and use that rather than rounding size up to an alignment boundary. > > Not exactly. Consider the unpatched code. At the beginning we > have some amount of space to be allocated on the stack at runtime > ("SSIZE"), some requested alignment for it ("SALIGN"). > > get_dynamic_stack_size() first calculates the space needed for run > time alignment: > > SIZE = SSIZE + SALIGN - 1 > > Then it calls round_push() to add *another* chunk of memory to the > allocation size to be able to align it to the required stack slot > alignment ("SLOTALIGN") at run time. > > SIZE = SIZE + SLOTALIGN - 1 > = SSIZE + (SALIGN - 1) + (SLOTALIGN - 1) > > Now it has added two chunks of memory but alignment is only done > once. With the patch it just adds the maximum of (SALIGN - 1) and > (SLOTALIGN - 1), not both. Thinking about it, the "round_push" > stuff is a very complicated way of saying "add max(A, B)". Now I see it. Thanks, that helped a ton. > > I'd volunteer to clean this up more, but preferrably when the two > pending patches are in. The current code is a real brain-twister. I'd be all for such cleanups after we wrap up the pending patches. It's certainly a rats nest of code right now. This patch is fine for the trunk. Thanks for your patience. jeff ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2][v3] Drop excess size used for run time allocated stack variables. 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 0 siblings, 1 reply; 6+ messages in thread From: Dominik Vogt @ 2016-07-22 12:02 UTC (permalink / raw) To: Jeff Law; +Cc: gcc-patches, Andreas Krebbel On Thu, Jul 21, 2016 at 02:07:05PM -0600, Jeff Law wrote: > On 06/23/2016 03:57 AM, Dominik Vogt wrote: > >>and use that rather than rounding size up to an alignment boundary. > > > >Not exactly. Consider the unpatched code. At the beginning we > >have some amount of space to be allocated on the stack at runtime > >("SSIZE"), some requested alignment for it ("SALIGN"). > > > >get_dynamic_stack_size() first calculates the space needed for run > >time alignment: > > > > SIZE = SSIZE + SALIGN - 1 > > > >Then it calls round_push() to add *another* chunk of memory to the > >allocation size to be able to align it to the required stack slot > >alignment ("SLOTALIGN") at run time. > > > > SIZE = SIZE + SLOTALIGN - 1 > > = SSIZE + (SALIGN - 1) + (SLOTALIGN - 1) > > > >Now it has added two chunks of memory but alignment is only done > >once. With the patch it just adds the maximum of (SALIGN - 1) and > >(SLOTALIGN - 1), not both. Thinking about it, the "round_push" > >stuff is a very complicated way of saying "add max(A, B)". > Now I see it. Thanks, that helped a ton. > > > > >I'd volunteer to clean this up more, but preferrably when the two > >pending patches are in. The current code is a real brain-twister. > I'd be all for such cleanups after we wrap up the pending patches. > It's certainly a rats nest of code right now. > > This patch is fine for the trunk. Thanks for your patience. 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 ^ 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
* 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-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
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).