[RFC] Change order of Fortran BLOCK_VARS (PR fortran/71687)

[RFC] Change order of Fortran BLOCK_VARS (PR fortran/71687)

[Jakub Jelinek]

Hi!

While usually the order of vars in block scope isn't that significant,
e.g. on the following testcase with -fopenmp -fstack-arrays it does matter
if we have local declaration of a temporary and then local declaration of
a VLA that uses that other temporary as the TYPE_SIZE_UNIT, or the other way
around (we ICE if we first see the VLA and firstprivatize the variable, only
to later on discover it is a local var).  pushdecl is called on them in the
right order (scalar temporary first, then VLA that uses it), but the names
chain is LIFO.
Actually, with gfc_merge_block_scope it seems we end up in quite randomish
order.
So, is it intentional that pushdecl / getdecls acts like a LIFO?
Though, it seems user vars are pushdecled in the reverse order of
declarations, but gfc_add_decl_to_function is called in the user declared
order, so perhaps for the following patch we'd also need to
decl = nreverse (saved_function_decls);
in gfc_generate_function_code and similarly in gfc_process_block_locals
decl = nreverse (saved_local_decls);

What do you think about this?

Note, nreverse is fairly cheap, it is linear in the chain length.

2016-06-30  Jakub Jelinek  <jakub@redhat.com>

	PR fortran/71687
	* f95-lang.c (struct binding_level): Add reversed field.
	(clear_binding_level): Adjust initializer.
	(getdecls): If reversed is clear, set it and nreverse the names
	chain before returning it.
	(poplevel): Use getdecls.

	* gfortran.dg/gomp/pr71687.f90: New test.

--- gcc/fortran/f95-lang.c.jj	2016-06-29 10:46:33.000000000 +0200
+++ gcc/fortran/f95-lang.c	2016-06-30 17:11:44.350240191 +0200
@@ -286,6 +286,9 @@ binding_level {
   tree blocks;
   /* The binding level containing this one (the enclosing binding level).  */
   struct binding_level *level_chain;
+  /* True if nreverse has been already called on names; if false, names
+     are ordered from newest declaration to oldest one.  */
+  bool reversed;
 };
 
 /* The binding level currently in effect.  */
@@ -296,7 +299,7 @@ static GTY(()) struct binding_level *cur
 static GTY(()) struct binding_level *global_binding_level;
 
 /* Binding level structures are initialized by copying this one.  */
-static struct binding_level clear_binding_level = { NULL, NULL, NULL };
+static struct binding_level clear_binding_level = { NULL, NULL, NULL, false };
 
 
 /* Return true if we are in the global binding level.  */
@@ -310,6 +313,11 @@ global_bindings_p (void)
 tree
 getdecls (void)
 {
+  if (!current_binding_level->reversed)
+    {
+      current_binding_level->reversed = true;
+      current_binding_level->names = nreverse (current_binding_level->names);
+    }
   return current_binding_level->names;
 }
 
@@ -347,7 +355,7 @@ poplevel (int keep, int functionbody)
      binding level that we are about to exit and which is returned by this
      routine.  */
   tree block_node = NULL_TREE;
-  tree decl_chain = current_binding_level->names;
+  tree decl_chain = getdecls ();
   tree subblock_chain = current_binding_level->blocks;
   tree subblock_node;
 
--- gcc/testsuite/gfortran.dg/gomp/pr71687.f90.jj	2016-06-30 17:12:27.279702475 +0200
+++ gcc/testsuite/gfortran.dg/gomp/pr71687.f90	2016-06-30 17:12:53.480374296 +0200
@@ -0,0 +1,11 @@
+! PR fortran/71687
+! { dg-do compile }
+! { dg-additional-options "-fstack-arrays -O2" }
+
+subroutine s (n, x)
+   integer :: n
+   real :: x(n)
+!$omp parallel
+   x(1:n) = x(n:1:-1)
+!$omp end parallel
+end


	Jakub

[RFC] Change order of Fortran BLOCK_VARS (PR fortran/71687, take 2)

[Jakub Jelinek]

On Thu, Jun 30, 2016 at 08:06:54PM +0200, Jakub Jelinek wrote:
> So, is it intentional that pushdecl / getdecls acts like a LIFO?
> Though, it seems user vars are pushdecled in the reverse order of
> declarations, but gfc_add_decl_to_function is called in the user declared
> order, so perhaps for the following patch we'd also need to
> decl = nreverse (saved_function_decls);
> in gfc_generate_function_code and similarly in gfc_process_block_locals
> decl = nreverse (saved_local_decls);

Here is an updated patch with the above mentioned two calls to nreverse
added.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2016-07-01  Jakub Jelinek  <jakub@redhat.com>

	PR fortran/71687
	* f95-lang.c (struct binding_level): Add reversed field.
	(clear_binding_level): Adjust initializer.
	(getdecls): If reversed is clear, set it and nreverse the names
	chain before returning it.
	(poplevel): Use getdecls.
	* trans-decl.c (gfc_generate_function_code, gfc_process_block_locals):
	Use nreverse to pushdecl decls in the declaration order.

	* gfortran.dg/gomp/pr71687.f90: New test.

--- gcc/fortran/f95-lang.c.jj	2016-06-30 19:38:38.296737997 +0200
+++ gcc/fortran/f95-lang.c	2016-07-01 12:12:24.821306073 +0200
@@ -286,6 +286,9 @@ binding_level {
   tree blocks;
   /* The binding level containing this one (the enclosing binding level).  */
   struct binding_level *level_chain;
+  /* True if nreverse has been already called on names; if false, names
+     are ordered from newest declaration to oldest one.  */
+  bool reversed;
 };
 
 /* The binding level currently in effect.  */
@@ -296,7 +299,7 @@ static GTY(()) struct binding_level *cur
 static GTY(()) struct binding_level *global_binding_level;
 
 /* Binding level structures are initialized by copying this one.  */
-static struct binding_level clear_binding_level = { NULL, NULL, NULL };
+static struct binding_level clear_binding_level = { NULL, NULL, NULL, false };
 
 
 /* Return true if we are in the global binding level.  */
@@ -310,6 +313,11 @@ global_bindings_p (void)
 tree
 getdecls (void)
 {
+  if (!current_binding_level->reversed)
+    {
+      current_binding_level->reversed = true;
+      current_binding_level->names = nreverse (current_binding_level->names);
+    }
   return current_binding_level->names;
 }
 
@@ -347,7 +355,7 @@ poplevel (int keep, int functionbody)
      binding level that we are about to exit and which is returned by this
      routine.  */
   tree block_node = NULL_TREE;
-  tree decl_chain = current_binding_level->names;
+  tree decl_chain = getdecls ();
   tree subblock_chain = current_binding_level->blocks;
   tree subblock_node;
 
--- gcc/fortran/trans-decl.c.jj	2016-05-09 10:20:26.000000000 +0200
+++ gcc/fortran/trans-decl.c	2016-07-01 12:25:30.863415372 +0200
@@ -6277,7 +6277,7 @@ gfc_generate_function_code (gfc_namespac
 			gfc_finish_block (&cleanup));
 
   /* Add all the decls we created during processing.  */
-  decl = saved_function_decls;
+  decl = nreverse (saved_function_decls);
   while (decl)
     {
       tree next;
@@ -6469,7 +6469,7 @@ gfc_process_block_locals (gfc_namespace*
   if (flag_coarray == GFC_FCOARRAY_LIB && has_coarray_vars)
     generate_coarray_init (ns);
 
-  decl = saved_local_decls;
+  decl = nreverse (saved_local_decls);
   while (decl)
     {
       tree next;
--- gcc/testsuite/gfortran.dg/gomp/pr71687.f90.jj	2016-07-01 12:12:24.821306073 +0200
+++ gcc/testsuite/gfortran.dg/gomp/pr71687.f90	2016-07-01 12:12:24.821306073 +0200
@@ -0,0 +1,11 @@
+! PR fortran/71687
+! { dg-do compile }
+! { dg-additional-options "-fstack-arrays -O2" }
+
+subroutine s (n, x)
+   integer :: n
+   real :: x(n)
+!$omp parallel
+   x(1:n) = x(n:1:-1)
+!$omp end parallel
+end

	Jakub

Re: [RFC] Change order of Fortran BLOCK_VARS (PR fortran/71687, take 2)

[Mikael Morin]

Le 01/07/2016 16:58, Jakub Jelinek a écrit :
> On Thu, Jun 30, 2016 at 08:06:54PM +0200, Jakub Jelinek wrote:
>> So, is it intentional that pushdecl / getdecls acts like a LIFO?
>> Though, it seems user vars are pushdecled in the reverse order of
>> declarations, but gfc_add_decl_to_function is called in the user declared
>> order, so perhaps for the following patch we'd also need to
>> decl = nreverse (saved_function_decls);
>> in gfc_generate_function_code and similarly in gfc_process_block_locals
>> decl = nreverse (saved_local_decls);
>
> Here is an updated patch with the above mentioned two calls to nreverse
> added.
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
Hello,

I'm a bit surprised that it has worked so well so far.
We should probably switch at some point to using a vec to collect decls 
without headache.  In the mean time, the patch is OK.  Thanks.

Mikael