| Message ID | 5582A8FA.5010002@sfr.fr |
|---|---|
| State | New |
| Headers | show |
On Thu, Jun 18, 2015 at 01:18:18PM +0200, Mikael Morin wrote: > I'm proposing here a fix for an OpenMP ICE regression introduced by me > at http://gcc.gnu.org/r221586 > > That revision changed the order in which procedures are resolved, making > it possible for a procedure to be resolved from within an OpenMP > construct body. As the OpenMP constructs set some global state, the > procedure's do loop were registered as OpenMP-managed loops. > > The attached patch clears the OpenMP state upon gfc_resolve entry > and restores it upon return. I don't know the OpenMP code very well, > but it seems reasonable to me to do that. > > Regression-tested on x86_64-linux. OK for trunk/5 ? I just wonder, the resolve_global_procedure code didn't save just OpenMP state, but also e.g. gfc_derived_types. Isn't that a problem too with wherever you are now calling gfc_resolve from within resolving of some other procedure? > 2015-06-18 Mikael Morin <mikael@gcc.gnu.org> > > PR fortran/66549 > * resolve.c (resolve_global_procedure): Don't save and restore > OpenMP state around the call to gfc_resolve. > (gfc_resolve): Save OpenMP state on entry and restore it on return. > > 2015-06-18 Mikael Morin <mikael@gcc.gnu.org> > > PR fortran/66549 > * gfortran.dg/gomp/omp_parallel_1.f90: New file. Jakub
Le 18/06/2015 13:35, Jakub Jelinek a écrit : > On Thu, Jun 18, 2015 at 01:18:18PM +0200, Mikael Morin wrote: >> I'm proposing here a fix for an OpenMP ICE regression introduced by me >> at http://gcc.gnu.org/r221586 >> >> That revision changed the order in which procedures are resolved, making >> it possible for a procedure to be resolved from within an OpenMP >> construct body. As the OpenMP constructs set some global state, the >> procedure's do loop were registered as OpenMP-managed loops. >> >> The attached patch clears the OpenMP state upon gfc_resolve entry >> and restores it upon return. I don't know the OpenMP code very well, >> but it seems reasonable to me to do that. >> >> Regression-tested on x86_64-linux. OK for trunk/5 ? > > I just wonder, the resolve_global_procedure code didn't save just > OpenMP state, but also e.g. gfc_derived_types. Isn't that a problem > too with wherever you are now calling gfc_resolve from within resolving > of some other procedure? > Hmm, for gfc_derived_types at least, I would say no. gfc_derived_types seems to be a per root namespace global, in other words shared between contained procedures, so it should not be saved/restored upon resolution of sibling or internal procedures. And I don't think it's possible to escape one root namespace; use-associated symbols are not shared with the corresponding module symbols for example. So we are (quite) safe here, I think. To be honest, I'm not at ease with these globals, both openmp and derived_types, and I'm not fond of starting resolution of a different procedure in the middle of nowhere. I can try to produce a patch that walks the procedure code to pick the needed information instead of doing full procedure resolutions at arbitrary places, if that seems worth it. Or try to remove gfc_derived_types. But IMHO the patch proposed here makes sense in any case. Mikael
On Thu, Jun 18, 2015 at 08:26:19PM +0200, Mikael Morin wrote: > Hmm, for gfc_derived_types at least, I would say no. > gfc_derived_types seems to be a per root namespace global, in other > words shared between contained procedures, so it should not be > saved/restored upon resolution of sibling or internal procedures. > And I don't think it's possible to escape one root namespace; > use-associated symbols are not shared with the corresponding module > symbols for example. > So we are (quite) safe here, I think. Ok. > To be honest, I'm not at ease with these globals, both openmp and > derived_types, and I'm not fond of starting resolution of a different > procedure in the middle of nowhere. I can try to produce a patch that > walks the procedure code to pick the needed information instead of doing > full procedure resolutions at arbitrary places, if that seems worth it. > Or try to remove gfc_derived_types. But IMHO the patch proposed here > makes sense in any case. If the patch passes regtest (both check-gfortran and libgomp fortran.exp), then it is ok for trunk/5 branch. Jakub
diff --git a/gcc/fortran/resolve.c b/gcc/fortran/resolve.c index e615cc6..23bee8f 100644 --- a/gcc/fortran/resolve.c +++ b/gcc/fortran/resolve.c @@ -2384,14 +2384,11 @@ resolve_global_procedure (gfc_symbol *sym, locus *where, if (!gsym->ns->resolved) { gfc_dt_list *old_dt_list; - struct gfc_omp_saved_state old_omp_state; /* Stash away derived types so that the backend_decls do not get mixed up. */ old_dt_list = gfc_derived_types; gfc_derived_types = NULL; - /* And stash away openmp state. */ - gfc_omp_save_and_clear_state (&old_omp_state); gfc_resolve (gsym->ns); @@ -2401,8 +2398,6 @@ resolve_global_procedure (gfc_symbol *sym, locus *where, /* Restore the derived types of this namespace. */ gfc_derived_types = old_dt_list; - /* And openmp state. */ - gfc_omp_restore_state (&old_omp_state); } /* Make sure that translation for the gsymbol occurs before @@ -15071,6 +15066,7 @@ gfc_resolve (gfc_namespace *ns) { gfc_namespace *old_ns; code_stack *old_cs_base; + struct gfc_omp_saved_state old_omp_state; if (ns->resolved) return; @@ -15079,6 +15075,11 @@ gfc_resolve (gfc_namespace *ns) old_ns = gfc_current_ns; old_cs_base = cs_base; + /* As gfc_resolve can be called during resolution of an OpenMP construct + body, we should clear any state associated to it, so that say NS's + DO loops are not interpreted as OpenMP loops. */ + gfc_omp_save_and_clear_state (&old_omp_state); + resolve_types (ns); component_assignment_level = 0; resolve_codes (ns); @@ -15088,4 +15089,6 @@ gfc_resolve (gfc_namespace *ns) ns->resolved = 1; gfc_run_passes (ns); + + gfc_omp_restore_state (&old_omp_state); }