| Message ID | 4E994C14.30008@netcologne.de |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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
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;
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