Patchwork [Fortran] Fix PR 50690

login
register
mail settings
Submitter Thomas Koenig
Date Oct. 15, 2011, 9:02 a.m.
Message ID <4E994C14.30008@netcologne.de>
Download mbox | patch
Permalink /patch/119960/
State New
Headers show

Comments

Thomas Koenig - Oct. 15, 2011, 9:02 a.m.
Hello world,

here is a fix for PR 50690, pretty self-explanatory.  Regression-tested.
OK for trunk?

	Thomas


2011-10-15  Thomas Koenig  <tkoenig@gcc.gnu.org>

         PR fortran/50690
         * frontend-passes.c (omp_level):  New variable.
         (create_var):  If we are within an OMP block, put
         the variable in the enclosing namespace and set
         the threadprivate attribute.
         (optimize_namespace):  Initialize omp_level.
         (gfc_code_walker):  Keep track of omp level.

2011-10-15  Thomas Koenig  <tkoenig@gcc.gnu.org>

         PR fortran/50690
         * gfortran.dg/gomp/workshare2.f90:  New test.
! { dg-do run }
! { dg-options "-ffrontend-optimize" }
! PR 50690 - this used to ICE because workshare could not handle
! BLOCKs.
program foo
  implicit none
  real, parameter :: eps = 3e-7
  integer :: i
  real :: A(10), B(5), C(10)
  B(1) = 3.344
  call random_number(a)
  c = a
  !$omp parallel default(shared)
  !$omp workshare
   A(:) = A(:)*cos(B(1))+A(:)*cos(B(1))
  !$omp end workshare nowait
  !$omp end parallel ! sync is implied here
  c = c*cos(b(1)) + c*cos(b(1))
  if (any(abs(a-c) > eps)) call abort
end program foo
Jakub Jelinek - Oct. 15, 2011, 11:36 a.m.
On Sat, Oct 15, 2011 at 11:02:12AM +0200, Thomas Koenig wrote:
> Hello world,
> 
> here is a fix for PR 50690, pretty self-explanatory.  Regression-tested.
> OK for trunk?

This looks wrong.  Threadprivate variables aren't completely cheap,
much better would be an automatic variable instead.
As Fortran FE IL doesn't have block local variables, I guess you want
to create the var at function scope and add a private(that_temporary)
clause to the nearest enclosing OpenMP directive. 

	Jakub
Thomas Koenig - Oct. 15, 2011, 12:04 p.m.
Am 15.10.2011 13:36, schrieb Jakub Jelinek:

> This looks wrong.  Threadprivate variables aren't completely cheap,
> much better would be an automatic variable instead.
> As Fortran FE IL doesn't have block local variables, I guess you want
> to create the var at function scope and add a private(that_temporary)
> clause to the nearest enclosing OpenMP directive.

Actually, Fortran does have BLOCK local variables, which I used in
the first place. This caused the ICE with workshare, because BLOCK
is not supported for workshare.

I'll work on your suggestion, though.

Thanks!

	Thomas
Tobias Burnus - Oct. 15, 2011, 4:36 p.m.
Am 15.10.2011 14:04, schrieb Thomas Koenig:
> Am 15.10.2011 13:36, schrieb Jakub Jelinek:
>> This looks wrong.  Threadprivate variables aren't completely cheap,
>> much better would be an automatic variable instead.
>> As Fortran FE IL doesn't have block local variables, I guess you want
>> to create the var at function scope and add a private(that_temporary)
>> clause to the nearest enclosing OpenMP directive.
>
> Actually, Fortran does have BLOCK local variables,

(Those were added with Fortran 2008 and in GCC 4.5)

> which I used in the first place. This caused the ICE with workshare, 
> because BLOCK
> is not supported for workshare.
>
> I'll work on your suggestion, though.

I think that's the wrong solution to the problem: I think it makes more 
sense to fix the handling for OMP workshare to handle BLOCK correctly 
rather than to move the variable to the function scope and then to 
fiddle with the private attribute. (By the way, an interesting question 
would be how the next [4.0] (or a next) OpenMP version will handle 
Fortran 2003 and 2008 features.)

Tobias
Jakub Jelinek - Oct. 15, 2011, 5:19 p.m.
On Sat, Oct 15, 2011 at 06:36:55PM +0200, Tobias Burnus wrote:
> I think that's the wrong solution to the problem: I think it makes
> more sense to fix the handling for OMP workshare to handle BLOCK
> correctly rather than to move the variable to the function scope and
> then to fiddle with the private attribute. (By the way, an

I don't agree here.  Until we know if OpenMP will allow or disallow
BLOCK in workshare and what the semantics for it should be, we shouldn't
allow it.  OpenMP <= 3.1 has very tight rules on what is allowed in
workshare, it is verified during resolving and during translation
we rely on it still being present.

	Jakub
Jakub Jelinek - Oct. 15, 2011, 5:24 p.m.
On Sat, Oct 15, 2011 at 07:19:43PM +0200, Jakub Jelinek wrote:
> On Sat, Oct 15, 2011 at 06:36:55PM +0200, Tobias Burnus wrote:
> > I think that's the wrong solution to the problem: I think it makes
> > more sense to fix the handling for OMP workshare to handle BLOCK
> > correctly rather than to move the variable to the function scope and
> > then to fiddle with the private attribute. (By the way, an
> 
> I don't agree here.  Until we know if OpenMP will allow or disallow
> BLOCK in workshare and what the semantics for it should be, we shouldn't
> allow it.  OpenMP <= 3.1 has very tight rules on what is allowed in
> workshare, it is verified during resolving and during translation
> we rely on it still being present.

Though, what could be done is just special case OpenMP workshare regions,
insert everything into BLOCK local vars unless in OpenMP workshare, in that
case put the BLOCK with the temporary around the workshare rather than
inside of it.  In the case of omp parallel workshare it would need
to go in between omp parallel and omp workshare.

	Jakub
Thomas Koenig - Oct. 16, 2011, 1:12 p.m.
Hi Tobias,

 > I think that's the wrong solution to the problem: I think it makes more
 > sense to fix the handling for OMP workshare to handle BLOCK correctly
 > rather than to move the variable to the function scope and then to
 > fiddle with the private attribute.

What is the reason for that?

I don't think there is a way to "correctly" handle BLOCK, since none
has been specified by the OMP standard yet.

I also tried the solution that Jakub proposed, but could not get it to
work. Since my time next week is limited, I am unassigning myself from
the PR for the moment.

If anybody has time to work on any of the alternatives, please feel
free.

     Thomas

Patch

Index: frontend-passes.c
===================================================================
--- frontend-passes.c	(Revision 179770)
+++ frontend-passes.c	(Arbeitskopie)
@@ -66,6 +66,11 @@  gfc_namespace *current_ns;
 
 static int forall_level;
 
+/* Keep track of the OMP statements, to make sure we can
+   mark variables introduced by optimizations as threadprivate.  */
+
+static int omp_level;
+
 /* Entry point - run all passes for a namespace.  So far, only an
    optimization pass is run.  */
 
@@ -245,9 +250,17 @@  create_var (gfc_expr * e)
   gfc_namespace *ns;
   int i;
 
-  /* If the block hasn't already been created, do so.  */
-  if (inserted_block == NULL)
+  /* If the block hasn't already been created, do so.  If we are within
+     an OMP construct, create the temporary variable in the current block.
+     This is to avoid problems with OMP workshare.  */
+
+  if (omp_level > 0)
     {
+      ns = current_ns;
+      changed_statement = current_code;
+    }
+  else if (inserted_block == NULL)
+    {
       inserted_block = XCNEW (gfc_code);
       inserted_block->op = EXEC_BLOCK;
       inserted_block->loc = (*current_code)->loc;
@@ -308,6 +321,9 @@  create_var (gfc_expr * e)
   symbol->attr.flavor = FL_VARIABLE;
   symbol->attr.referenced = 1;
   symbol->attr.dimension = e->rank > 0;
+  if (omp_level > 0)
+    symbol->attr.threadprivate = 1;
+
   gfc_commit_symbol (symbol);
 
   result = gfc_get_expr ();
@@ -504,6 +520,7 @@  optimize_namespace (gfc_namespace *ns)
 
   current_ns = ns;
   forall_level = 0;
+  omp_level = 0;
 
   gfc_code_walker (&ns->code, convert_do_while, dummy_expr_callback, NULL);
   gfc_code_walker (&ns->code, cfe_code, cfe_expr_0, NULL);
@@ -1143,11 +1160,13 @@  gfc_code_walker (gfc_code **c, walk_code_fn_t code
 	  gfc_code *b;
 	  gfc_actual_arglist *a;
 	  gfc_code *co;
+	  bool in_omp;
 
 	  /* There might be statement insertions before the current code,
 	     which must not affect the expression walker.  */
 
 	  co = *c;
+	  in_omp = false;
 
 	  switch (co->op)
 	    {
@@ -1326,6 +1345,9 @@  gfc_code_walker (gfc_code **c, walk_code_fn_t code
 	    case EXEC_OMP_WORKSHARE:
 	    case EXEC_OMP_END_SINGLE:
 	    case EXEC_OMP_TASK:
+
+	      in_omp = 1;
+	      omp_level ++;
 	      if (co->ext.omp_clauses)
 		{
 		  WALK_SUBEXPR (co->ext.omp_clauses->if_expr);
@@ -1352,6 +1374,9 @@  gfc_code_walker (gfc_code **c, walk_code_fn_t code
 	  if (co->op == EXEC_FORALL)
 	    forall_level --;
 
+	  if (in_omp)
+	    omp_level --;
+
 	}
     }
   return 0;