Re: [PATCH v5] Allocate constant size dynamic stack space in the prologue

[Andreas Krebbel]

> gcc/ChangeLog
> 
>   * cfgexpand.c (expand_stack_vars): Implement synamic stack space
>   allocation in the prologue.
>   * explow.c (get_dynamic_stack_base): New function to return an address
>   expression for the dynamic stack base.
>   (get_dynamic_stack_size): New function to do the required dynamic stack
>   space size calculations.
>   (allocate_dynamic_stack_space): Use new functions.
>   (align_dynamic_address): Move some code from
>   allocate_dynamic_stack_space to new function.
>   * explow.h (get_dynamic_stack_base, get_dynamic_stack_size): Export.
> gcc/testsuite/ChangeLog
> 
>   * gcc.target/s390/warn-dynamicstack-1.c: New test.
>   * gcc.dg/stack-usage-2.c (foo3): Adapt expected warning.
>   stack-layout-dynamic-1.c: New test.

Applied.  Thanks!

-Andreas-

Re: [PATCH v5] Allocate constant size dynamic stack space in the prologue

[Bernd Schmidt]

On 07/15/2016 02:22 PM, Dominik Vogt wrote:

Final version 6 with the stray comment removed (was a harmless
oversight).


Ok, let's put it in.


Bernd

Re: [PATCH v5] Allocate constant size dynamic stack space in the prologue

[Dominik Vogt]

Final version 6 with the stray comment removed (was a harmless
oversight).

Initial description of the patch:
https://gcc.gnu.org/ml/gcc-patches/2015-11/msg03052.html

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany
gcc/ChangeLog

* cfgexpand.c (expand_stack_vars): Implement synamic stack space
allocation in the prologue.
* explow.c (get_dynamic_stack_base): New function to return an address
expression for the dynamic stack base.
(get_dynamic_stack_size): New function to do the required dynamic stack
space size calculations.
(allocate_dynamic_stack_space): Use new functions.
(align_dynamic_address): Move some code from
allocate_dynamic_stack_space to new function.
* explow.h (get_dynamic_stack_base, get_dynamic_stack_size): Export.
gcc/testsuite/ChangeLog

* gcc.target/s390/warn-dynamicstack-1.c: New test.
* gcc.dg/stack-usage-2.c (foo3): Adapt expected warning.
stack-layout-dynamic-1.c: New test.
>From 03d09e6adacf582d52f53ca36cfa3c001ed444e5 Mon Sep 17 00:00:00 2001
From: Dominik Vogt 
Date: Wed, 25 Nov 2015 09:31:19 +0100
Subject: [PATCH] Allocate constant size dynamic stack space in the
 prologue ...

... and place it in the virtual stack vars area, if the platform supports it.
On S/390 this saves adjusting the stack pointer twice and forcing the frame
pointer into existence.  It also removes the warning with -mwarn-dynamicstack
that is triggered by cfun->calls_alloca == 1.

This fixes a problem with the Linux kernel which aligns the page structure to
16 bytes at run time using inefficient code and issuing a bogus warning.
---
 gcc/cfgexpand.c|  22 +-
 gcc/explow.c   | 225 ++---
 gcc/explow.h   |   8 +
 gcc/testsuite/gcc.dg/stack-layout-dynamic-1.c  |  14 ++
 gcc/testsuite/gcc.dg/stack-usage-2.c   |   4 +-
 .../gcc.target/s390/warn-dynamicstack-1.c  |  17 ++
 6 files changed, 206 insertions(+), 84 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/stack-layout-dynamic-1.c
 create mode 100644 gcc/testsuite/gcc.target/s390/warn-dynamicstack-1.c

diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index e4ddb3a..5046d61 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -1053,6 +1053,7 @@ expand_stack_vars (bool (*pred) (size_t), struct 
stack_vars_data *data)
   HOST_WIDE_INT large_size = 0, large_alloc = 0;
   rtx large_base = NULL;
   unsigned large_align = 0;
+  bool large_allocation_done = false;
   tree decl;
 
   /* Determine if there are any variables requiring "large" alignment.
@@ -1096,11 +1097,6 @@ expand_stack_vars (bool (*pred) (size_t), struct 
stack_vars_data *data)
  large_size &= -(HOST_WIDE_INT)alignb;
  large_size += stack_vars[i].size;
}
-
-  /* If there were any, allocate space.  */
-  if (large_size > 0)
-   large_base = allocate_dynamic_stack_space (GEN_INT (large_size), 0,
-  large_align, true);
 }
 
   for (si = 0; si < n; ++si)
@@ -1186,6 +1182,22 @@ expand_stack_vars (bool (*pred) (size_t), struct 
stack_vars_data *data)
  /* Large alignment is only processed in the last pass.  */
  if (pred)
continue;
+
+ /* If there were any variables requiring "large" alignment, allocate
+space.  */
+ if (large_size > 0 && ! large_allocation_done)
+   {
+ HOST_WIDE_INT loffset;
+ rtx large_allocsize;
+
+ large_allocsize = GEN_INT (large_size);
+ get_dynamic_stack_size (_allocsize, 0, large_align, NULL);
+ loffset = alloc_stack_frame_space
+   (INTVAL (large_allocsize),
+PREFERRED_STACK_BOUNDARY / BITS_PER_UNIT);
+ large_base = get_dynamic_stack_base (loffset, large_align);
+ large_allocation_done = true;
+   }
  gcc_assert (large_base != NULL);
 
  large_alloc += alignb - 1;
diff --git a/gcc/explow.c b/gcc/explow.c
index 09a0330..a345690 100644
--- a/gcc/explow.c
+++ b/gcc/explow.c
@@ -1146,82 +1146,55 @@ record_new_stack_level (void)
 update_sjlj_context ();
 }
 
-/* Return an rtx representing the address of an area of memory dynamically
-   pushed on the stack.
+/* Return an rtx doing runtime alignment to REQUIRED_ALIGN on TARGET.  */
+static rtx
+align_dynamic_address (rtx target, unsigned required_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, 

Re: [PATCH v5] Allocate constant size dynamic stack space in the prologue

[Dominik Vogt]

On Fri, Jul 15, 2016 at 01:51:51PM +0200, Bernd Schmidt wrote:
> On 07/14/2016 11:10 AM, Dominik Vogt wrote:
> 
> >-  if (flag_stack_usage_info)
> >-stack_usage_size += extra;
> >+  /*!!!*/
> >+  if (flag_stack_usage_info && pstack_usage_size)
> >+*pstack_usage_size += extra;
> 
> What does the comment mean?

It means that I wanted to look at that code location for some
reason and forgot to do so.  Need to check why I've put it in
there in the first place.

> Other than that it looks ok, although I still feel uneasy about the
> unexplained movement of code since v1 - next time you fix a bug in a
> patch, please document what happened.

Yep.

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany

Re: [PATCH v5] Allocate constant size dynamic stack space in the prologue

[Bernd Schmidt]

On 07/14/2016 11:10 AM, Dominik Vogt wrote:


-  if (flag_stack_usage_info)
-stack_usage_size += extra;
+  /*!!!*/
+  if (flag_stack_usage_info && pstack_usage_size)
+*pstack_usage_size += extra;


What does the comment mean? Whatever it is needs to be addressed and the 
comment removed.


Other than that it looks ok, although I still feel uneasy about the 
unexplained movement of code since v1 - next time you fix a bug in a 
patch, please document what happened.



Bernd

Re: [PATCH v5] Allocate constant size dynamic stack space in the prologue

[Dominik Vogt]

On Wed, Jul 13, 2016 at 04:12:36PM -0600, Jeff Law wrote:
> On 07/11/2016 05:44 AM, Dominik Vogt wrote:
> >On Thu, Jul 07, 2016 at 12:57:16PM +0100, Dominik Vogt wrote:
> >>On Wed, Jul 06, 2016 at 02:01:06PM +0200, Bernd Schmidt wrote:
> >>>There's one thing I don't quite understand and which seems to have
> >>>changed since v1:
> >>>
> >>>On 07/04/2016 02:19 PM, Dominik Vogt wrote:
> @@ -1099,8 +1101,10 @@ expand_stack_vars (bool (*pred) (size_t), struct 
> stack_vars_data *data)
> 
>   /* If there were any, allocate space.  */
>   if (large_size > 0)
> - large_base = allocate_dynamic_stack_space (GEN_INT (large_size), 0,
> -large_align, true);
> + {
> +   large_allocsize = GEN_INT (large_size);
> +   get_dynamic_stack_size (_allocsize, 0, large_align, NULL);
> + }
> }
> 
>   for (si = 0; si < n; ++si)
> @@ -1186,6 +1190,19 @@ expand_stack_vars (bool (*pred) (size_t), struct 
> stack_vars_data *data)
> /* Large alignment is only processed in the last pass.  */
> if (pred)
>   continue;
> +
> +   if (large_allocsize && ! large_allocation_done)
> + {
> +   /* Allocate space the virtual stack vars area in the
> +  prologue.  */
> +   HOST_WIDE_INT loffset;
> +
> +   loffset = alloc_stack_frame_space
> + (INTVAL (large_allocsize),
> +  PREFERRED_STACK_BOUNDARY / BITS_PER_UNIT);
> +   large_base = get_dynamic_stack_base (loffset, large_align);
> +   large_allocation_done = true;
> + }
> gcc_assert (large_base != NULL);
> 
> >>>
> >>>Why is this code split between the two places here?
> >>
> >>This is a bugfix from v1 to v2.
> >>I think I had to move this code to the second loop so that the
> >>space for dynamically aligned variables is allocated at the "end"
> >>of the space for stack variables.  I cannot remember what the bug
> >>was, but maybe it was that the variables with fixed and static
> >>alignment ended up at the same address.
> >>
> >>Anyway, now that the new allocation strategy is used
> >>unconditionally, it should be possible to move the
> >>get_dynamic_stack_size call down to the second loop and simplify
> >>the code somewhat.  I'll look into that.
> >
> >Version 5 with some code moved from the first loop to the second.
> -ENOPATCH

Oops.

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany
gcc/ChangeLog

* cfgexpand.c (expand_stack_vars): Implement synamic stack space
allocation in the prologue.
* explow.c (get_dynamic_stack_base): New function to return an address
expression for the dynamic stack base.
(get_dynamic_stack_size): New function to do the required dynamic stack
space size calculations.
(allocate_dynamic_stack_space): Use new functions.
(align_dynamic_address): Move some code from
allocate_dynamic_stack_space to new function.
* explow.h (get_dynamic_stack_base, get_dynamic_stack_size): Export.
gcc/testsuite/ChangeLog

* gcc.target/s390/warn-dynamicstack-1.c: New test.
* gcc.dg/stack-usage-2.c (foo3): Adapt expected warning.
stack-layout-dynamic-1.c: New test.
>From 254581388640af34bcff55105ec13555043b62e5 Mon Sep 17 00:00:00 2001
From: Dominik Vogt 
Date: Wed, 25 Nov 2015 09:31:19 +0100
Subject: [PATCH] Allocate constant size dynamic stack space in the
 prologue ...

... and place it in the virtual stack vars area, if the platform supports it.
On S/390 this saves adjusting the stack pointer twice and forcing the frame
pointer into existence.  It also removes the warning with -mwarn-dynamicstack
that is triggered by cfun->calls_alloca == 1.

This fixes a problem with the Linux kernel which aligns the page structure to
16 bytes at run time using inefficient code and issuing a bogus warning.
---
 gcc/cfgexpand.c|  22 +-
 gcc/explow.c   | 226 ++---
 gcc/explow.h   |   8 +
 gcc/testsuite/gcc.dg/stack-layout-dynamic-1.c  |  14 ++
 gcc/testsuite/gcc.dg/stack-usage-2.c   |   4 +-
 .../gcc.target/s390/warn-dynamicstack-1.c  |  17 ++
 6 files changed, 207 insertions(+), 84 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/stack-layout-dynamic-1.c
 create mode 100644 gcc/testsuite/gcc.target/s390/warn-dynamicstack-1.c

diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index e4ddb3a..5046d61 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -1053,6 +1053,7 @@ expand_stack_vars (bool (*pred) (size_t), struct stack_vars_data *data)
   HOST_WIDE_INT large_size = 0, large_alloc = 0;
   rtx large_base = NULL;
   unsigned large_align = 0;
+  bool large_allocation_done = false;
   tree decl;
 
   /* Determine if there are any variables requiring "large" alignment.
@@ 

Re: [PATCH v5] Allocate constant size dynamic stack space in the prologue

[Jeff Law]

On 07/11/2016 05:44 AM, Dominik Vogt wrote:

On Thu, Jul 07, 2016 at 12:57:16PM +0100, Dominik Vogt wrote:

On Wed, Jul 06, 2016 at 02:01:06PM +0200, Bernd Schmidt wrote:

There's one thing I don't quite understand and which seems to have
changed since v1:

On 07/04/2016 02:19 PM, Dominik Vogt wrote:

@@ -1099,8 +1101,10 @@ expand_stack_vars (bool (*pred) (size_t), struct 
stack_vars_data *data)

  /* If there were any, allocate space.  */
  if (large_size > 0)
-   large_base = allocate_dynamic_stack_space (GEN_INT (large_size), 0,
-  large_align, true);
+   {
+ large_allocsize = GEN_INT (large_size);
+ get_dynamic_stack_size (_allocsize, 0, large_align, NULL);
+   }
}

  for (si = 0; si < n; ++si)
@@ -1186,6 +1190,19 @@ expand_stack_vars (bool (*pred) (size_t), struct 
stack_vars_data *data)
  /* Large alignment is only processed in the last pass.  */
  if (pred)
continue;
+
+ if (large_allocsize && ! large_allocation_done)
+   {
+ /* Allocate space the virtual stack vars area in the
+prologue.  */
+ HOST_WIDE_INT loffset;
+
+ loffset = alloc_stack_frame_space
+   (INTVAL (large_allocsize),
+PREFERRED_STACK_BOUNDARY / BITS_PER_UNIT);
+ large_base = get_dynamic_stack_base (loffset, large_align);
+ large_allocation_done = true;
+   }
  gcc_assert (large_base != NULL);



Why is this code split between the two places here?


This is a bugfix from v1 to v2.
I think I had to move this code to the second loop so that the
space for dynamically aligned variables is allocated at the "end"
of the space for stack variables.  I cannot remember what the bug
was, but maybe it was that the variables with fixed and static
alignment ended up at the same address.

Anyway, now that the new allocation strategy is used
unconditionally, it should be possible to move the
get_dynamic_stack_size call down to the second loop and simplify
the code somewhat.  I'll look into that.


Version 5 with some code moved from the first loop to the second.

-ENOPATCH
jeff

Re: [PATCH v5] Allocate constant size dynamic stack space in the prologue

[Dominik Vogt]

On Thu, Jul 07, 2016 at 12:57:16PM +0100, Dominik Vogt wrote:
> On Wed, Jul 06, 2016 at 02:01:06PM +0200, Bernd Schmidt wrote:
> > There's one thing I don't quite understand and which seems to have
> > changed since v1:
> > 
> > On 07/04/2016 02:19 PM, Dominik Vogt wrote:
> > >@@ -1099,8 +1101,10 @@ expand_stack_vars (bool (*pred) (size_t), struct 
> > >stack_vars_data *data)
> > >
> > >   /* If there were any, allocate space.  */
> > >   if (large_size > 0)
> > >-  large_base = allocate_dynamic_stack_space (GEN_INT (large_size), 0,
> > >- large_align, true);
> > >+  {
> > >+large_allocsize = GEN_INT (large_size);
> > >+get_dynamic_stack_size (_allocsize, 0, large_align, NULL);
> > >+  }
> > > }
> > >
> > >   for (si = 0; si < n; ++si)
> > >@@ -1186,6 +1190,19 @@ expand_stack_vars (bool (*pred) (size_t), struct 
> > >stack_vars_data *data)
> > > /* Large alignment is only processed in the last pass.  */
> > > if (pred)
> > >   continue;
> > >+
> > >+if (large_allocsize && ! large_allocation_done)
> > >+  {
> > >+/* Allocate space the virtual stack vars area in the
> > >+   prologue.  */
> > >+HOST_WIDE_INT loffset;
> > >+
> > >+loffset = alloc_stack_frame_space
> > >+  (INTVAL (large_allocsize),
> > >+   PREFERRED_STACK_BOUNDARY / BITS_PER_UNIT);
> > >+large_base = get_dynamic_stack_base (loffset, large_align);
> > >+large_allocation_done = true;
> > >+  }
> > > gcc_assert (large_base != NULL);
> > >
> > 
> > Why is this code split between the two places here?
> 
> This is a bugfix from v1 to v2.
> I think I had to move this code to the second loop so that the
> space for dynamically aligned variables is allocated at the "end"
> of the space for stack variables.  I cannot remember what the bug
> was, but maybe it was that the variables with fixed and static
> alignment ended up at the same address.
> 
> Anyway, now that the new allocation strategy is used
> unconditionally, it should be possible to move the
> get_dynamic_stack_size call down to the second loop and simplify
> the code somewhat.  I'll look into that.

Version 5 with some code moved from the first loop to the second.

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany