Re: [Patch, Fortran] PR32359 OpenMP threadprivate: SAVE is implied by explicit initialization

[Tobias Burnus <burnus at net-b dot de>]

Daniel Franke wrote:
> any specific reason why you high-jacked that PR? I had it already assigned to me :)
>   
I somehow missed that :-(

> Comments to your patch: it fixes only the one issue with threadprivate,
>   
> Instead of fixes them one by one, I'd suggest to change the save-attribute in 
> in 'symbol_attribute' from a boolean to a tri-state, say: SAVE_NONE = 0, 
> SAVE_EXPLICIT, SAVE_IMPLICIT.

Granted, but attr.save is used only very rarely in the code and the use
seems to be ok. (I have to admit, I got lost in trans-array.c and
trans-common.c; I don't see whether attr.save or attr.save ==
SAVE_EXPLICIT is needed there. I think the change is ok with regards to
trans-*.c.)

However, your patch missed the following use of the "SAVE" statement:
   integer :: y
   save

How about something like the attached patch?

The testsuites libgomp & gfortran have no failures but:
gfortran.dg/module_md5_1.f90; after glancing at module.c I still don't
quite understand why, but I get the MD5 sum:
MD5:2acedc3d584fad6aae6248078b02aa38 instead of
MD5:18a257e13c90e3872b7b9400c2fc6e4b. As the distinction between
SAVE_IMPLICIT and SAVE_EXPLICIT is lost, when reading/writing a mod file
(the distinction should not be needed for use-associated symbols), I
don't see how the MD5 sum could possibly change.

Tobias

2007-06-16  Daniel Franke  <franke.daniel@gmail.com>
	    Tobias Burnus  <burnus@net-b.de>

	PR fortran/32359
	* gfortran.h (symbol_attribute): Change save attribute into an enum.
	* decl.c (add_init_expr_to_sym): Set it to SAVE_IMPLICIT.
	* symbol.c (gfc_add_save): Check for SAVE_EXPLICIT.
	* resolve.c (resolve_fl_variable): Check for SAVE_EXPLICIT.
	(resolve_symbol): Allow OMP threadprivate with
	initialization SAVEd and save_all variable.
	* trans-decl.c (gfc_finish_var_decl): Remove obsolete sym->value check.

2007-06-16  Tobias Burnus  <burnus@net-b.de>

	PR fortran/32359
	* testsuite/libgomp.fortran/pr32359.f90: New.


Index: gcc/fortran/symbol.c
===================================================================
--- gcc/fortran/symbol.c	(revision 125755)
+++ gcc/fortran/symbol.c	(working copy)
@@ -885,7 +885,7 @@ gfc_add_save (symbol_attribute *attr, co
       return FAILURE;
     }
 
-  if (attr->save)
+  if (attr->save == SAVE_EXPLICIT)
     {
 	if (gfc_notify_std (GFC_STD_LEGACY, 
 			    "Duplicate SAVE attribute specified at %L",
@@ -894,7 +894,7 @@ gfc_add_save (symbol_attribute *attr, co
 	  return FAILURE;
     }
 
-  attr->save = 1;
+  attr->save = SAVE_EXPLICIT;
   return check_conflict (attr, name, where);
 }
 
Index: gcc/fortran/decl.c
===================================================================
--- gcc/fortran/decl.c	(revision 125755)
+++ gcc/fortran/decl.c	(working copy)
@@ -1021,6 +1021,7 @@ add_init_expr_to_sym (const char *name, 
 	}
 
       sym->value = init;
+      sym->attr.save = SAVE_IMPLICIT;
       *initp = NULL;
     }
 
Index: gcc/fortran/gfortran.h
===================================================================
--- gcc/fortran/gfortran.h	(revision 125755)
+++ gcc/fortran/gfortran.h	(working copy)
@@ -291,6 +291,12 @@ typedef enum ifsrc
 }
 ifsrc;
 
+/* Whether a SAVE attribute was set explicitly or implictly.  */
+typedef enum save_state
+{ SAVE_NONE = 0, SAVE_EXPLICIT, SAVE_IMPLICIT
+}
+save_state;
+
 /* Strings for all symbol attributes.  We use these for dumping the
    parse tree, in error messages, and also when reading and writing
    modules.  In symbol.c.  */
@@ -558,10 +564,12 @@ typedef struct
 {
   /* Variable attributes.  */
   unsigned allocatable:1, dimension:1, external:1, intrinsic:1,
-    optional:1, pointer:1, save:1, target:1, value:1, volatile_:1,
+    optional:1, pointer:1, target:1, value:1, volatile_:1,
     dummy:1, result:1, assign:1, threadprivate:1, not_always_present:1,
     implied_index:1;
 
+  ENUM_BITFIELD (save_state) save:2;
+
   unsigned data:1,		/* Symbol is named in a DATA statement.  */
     protected:1,		/* Symbol has been marked as protected.  */
     use_assoc:1,		/* Symbol has been use-associated.  */
Index: gcc/fortran/resolve.c
===================================================================
--- gcc/fortran/resolve.c	(revision 125755)
+++ gcc/fortran/resolve.c	(working copy)
@@ -5780,8 +5780,9 @@ resolve_fl_variable (gfc_symbol *sym, in
 	    }
 	}
 
-      /* Also, they must not have the SAVE attribute.  */
-      if (flag && sym->attr.save)
+      /* Also, they must not have the SAVE attribute;
+	 SAVE_IMPLICIT is checked below.  */
+      if (flag && sym->attr.save == SAVE_EXPLICIT)
 	{
 	  gfc_error (auto_save_msg, sym->name, &sym->declared_at);
 	  return FAILURE;
@@ -6428,7 +6429,7 @@ resolve_symbol (gfc_symbol *sym)
     gfc_resolve (sym->formal_ns);
 
   /* Check threadprivate restrictions.  */
-  if (sym->attr.threadprivate && !sym->attr.save
+  if (sym->attr.threadprivate && !sym->attr.save && !sym->ns->save_all
       && (!sym->attr.in_common
 	  && sym->module == NULL
 	  && (sym->ns->proc_name == NULL
Index: libgomp/testsuite/libgomp.fortran/pr32359.f90
===================================================================
--- libgomp/testsuite/libgomp.fortran/pr32359.f90	(revision 0)
+++ libgomp/testsuite/libgomp.fortran/pr32359.f90	(revision 0)
@@ -0,0 +1,34 @@
+! { dg-do compile }
+!
+! PR fortran/32359
+! Contributed by Bill Long <longb@cray.com>
+
+subroutine test
+      use omp_lib
+      implicit none
+      integer, parameter :: NT = 4
+      integer :: a
+      save
+!$omp threadprivate(a)
+      a = 1
+
+!$    call omp_set_num_threads(NT)
+!$omp parallel
+      print *, omp_get_thread_num(), a
+!$omp end parallel
+
+end subroutine test
+
+! Derived from OpenMP test omp1/F2_6_2_8_5i.f90
+      use omp_lib
+      implicit none
+      integer, parameter :: NT = 4
+      integer :: a = 1
+!$omp threadprivate(a)
+
+!$    call omp_set_num_threads(NT)
+!$omp parallel
+      print *, omp_get_thread_num(), a
+!$omp end parallel
+
+      END