| Message ID | 58c58de5-97d0-ea91-b013-ede6af4d16f6@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | [nvptx,committed] Unify C/Fortran routine handling in nvptx_goacc_validate_dims | expand |
Hi! On Mon, 17 Dec 2018 22:46:50 +0100, Tom de Vries <tdevries@suse.de> wrote: > [ was: Re: [nvptx] vector length patch series ] > > On 14-12-18 20:58, Tom de Vries wrote: > >> 0004-openacc-Make-GFC-default-to-1-for-OpenACC-routine-di.patch > > If I remove this, I run into ICEs in the compiler, but I think that > > means we need to understand and fix that ICE, instead of concluding that > > we need this patch. It looks completely unrelated. > > Indeed this > (0004-openacc-Make-GFC-default-to-1-for-OpenACC-routine-di.patch) patch > is unrelated to the vector length functionality. > > However, it fixes a problem in the Fortran front-end which has as > consequence that C and Fortran routines look the same in > nvptx_goacc_validate_dims, which is a good thing to have. > > However, the upstreaming of the patch seems to be stuck, so I've > committed an nvptx workaround patch that has the same effect, allowing > us to drop it > (0004-openacc-Make-GFC-default-to-1-for-OpenACC-routine-di.patch) from > the patch series. ACK, thanks. > [nvptx] Unify C/Fortran routine handling in nvptx_goacc_validate_dims > > The Fortran front-end has a bug (PR72741) that means what when > nvptx_goacc_validate_dims is called for a Fortran routine, the dims parameter > is not the same as it would have been if the function would have been called for > an equivalent C routine. > > Work around this bug by overriding the dims parameter for routines, allowing the > function to handle routines in Fortran and C the same. I have now finally identified the relevant changes (scattered over several commits on the OpenACC development branch, each of these trying to do too many things at once, but also incompletely...), and then have rewritten most of it anyway, into a more pleasant form, and now committed to trunk in r269105 "[PR72741] Use 'oacc_build_routine_dims' for Fortran OpenACC 'routine' directives, too", as attached. Grüße Thomas From 1d740b07b3ba5b15b7ece7fdb25236e32251131a Mon Sep 17 00:00:00 2001 From: tschwinge <tschwinge@138bc75d-0d04-0410-961f-82ee72b054a4> Date: Fri, 22 Feb 2019 10:50:35 +0000 Subject: [PATCH 4/9] [PR72741] Use 'oacc_build_routine_dims' for Fortran OpenACC 'routine' directives, too ... instead of having an incomplete local implementation. With these changes in place, we can then also revert the work-around r267213 "[nvptx] Unify C/Fortran routine handling in nvptx_goacc_validate_dims". gcc/fortran/ PR fortran/72741 * gfortran.h (oacc_routine_lop): New enum. (symbol_attribute): Use it. * openmp.c (gfc_oacc_routine_dims): Replace with... (gfc_oacc_routine_lop): ... this new function. (gfc_match_oacc_routine): Adjust. * trans-decl.c (add_attributes_to_decl): Likewise. gcc/ PR fortran/72741 * omp-general.c (oacc_replace_fn_attrib): Mostly split out into... (oacc_replace_fn_attrib_attr): ... this new function. * omp-general.h (oacc_replace_fn_attrib_attr): New prototype. * config/nvptx/nvptx.c (nvptx_goacc_validate_dims_1): Revert workaround. gcc/testsuite/ PR fortran/72741 * gfortran.dg/goacc/classify-routine.f95: Adjust. git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@269105 138bc75d-0d04-0410-961f-82ee72b054a4 --- gcc/ChangeLog | 8 ++++ gcc/config/nvptx/nvptx.c | 35 ---------------- gcc/fortran/ChangeLog | 11 +++++ gcc/fortran/gfortran.h | 13 +++++- gcc/fortran/openmp.c | 41 +++++++++++-------- gcc/fortran/trans-decl.c | 34 ++++++++++----- gcc/omp-general.c | 18 ++++++-- gcc/omp-general.h | 1 + gcc/testsuite/ChangeLog | 3 ++ .../gfortran.dg/goacc/classify-routine.f95 | 4 +- 10 files changed, 99 insertions(+), 69 deletions(-) diff --git a/gcc/ChangeLog b/gcc/ChangeLog index 4745a1999d9..f14cbbce477 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,11 @@ +2019-02-22 Thomas Schwinge <thomas@codesourcery.com> + + PR fortran/72741 + * omp-general.c (oacc_replace_fn_attrib): Mostly split out into... + (oacc_replace_fn_attrib_attr): ... this new function. + * omp-general.h (oacc_replace_fn_attrib_attr): New prototype. + * config/nvptx/nvptx.c (nvptx_goacc_validate_dims_1): Revert workaround. + 2019-02-22 Kyrylo Tkachov <kyrylo.tkachov@arm.com> * config/arm/arm-cpus.in (ares): Rename to... diff --git a/gcc/config/nvptx/nvptx.c b/gcc/config/nvptx/nvptx.c index 23459e1c6f4..424b43ac8b4 100644 --- a/gcc/config/nvptx/nvptx.c +++ b/gcc/config/nvptx/nvptx.c @@ -5577,41 +5577,6 @@ nvptx_goacc_validate_dims_1 (tree decl, int dims[], int fn_level, unsigned used) else gcc_unreachable (); - if (routine_p) - { - /* OpenACC routines in C arrive here with the following attributes - (omitting the 'omp declare target'): - seq : __attribute__((oacc function (0 1, 0 1, 0 1))) - vector: __attribute__((oacc function (0 1, 0 1, 1 0))) - worker: __attribute__((oacc function (0 1, 1 0, 1 0))) - gang : __attribute__((oacc function (1 0, 1 0, 1 0))) - - If we take f.i. the oacc function attribute of the worker routine - (0 1, 1 0, 1 0), then: - - the slice (0, 1, 1) is interpreted by oacc_fn_attrib_level as - meaning: worker routine, that is: - - can't contain gang loop (0), - - can contain worker loop (1), - - can contain vector loop (1). - - the slice (1, 0, 0) is interpreted by oacc_validate_dims as the - dimensions: gang: 1, worker: 0, vector: 0. - - OTOH, routines in Fortran arrive here with these attributes: - seq : __attribute__((oacc function (0 0, 0 0, 0 0))) - vector: __attribute__((oacc function (0 0, 0 0, 1 0))) - worker: __attribute__((oacc function (0 0, 1 0, 1 0))) - gang : __attribute__((oacc function (1 0, 1 0, 1 0))) - that is, the same as for C but with the dimensions set to 0. - - This is due to a bug in the Fortran front-end: PR72741. Work around - this bug by forcing the dimensions to be the same in Fortran as for C, - to be able to handle C and Fortran routines uniformly in this - function. */ - dims[GOMP_DIM_VECTOR] = fn_level > GOMP_DIM_VECTOR ? 1 : 0; - dims[GOMP_DIM_WORKER] = fn_level > GOMP_DIM_WORKER ? 1 : 0; - dims[GOMP_DIM_GANG] = fn_level > GOMP_DIM_GANG ? 1 : 0; - } - if (oacc_min_dims_p) { gcc_assert (dims[GOMP_DIM_VECTOR] == 1); diff --git a/gcc/fortran/ChangeLog b/gcc/fortran/ChangeLog index 74a6890ed70..0eb860449ca 100644 --- a/gcc/fortran/ChangeLog +++ b/gcc/fortran/ChangeLog @@ -1,3 +1,14 @@ +2019-02-22 Thomas Schwinge <thomas@codesourcery.com> + Cesar Philippidis <cesar@codesourcery.com> + + PR fortran/72741 + * gfortran.h (oacc_routine_lop): New enum. + (symbol_attribute): Use it. + * openmp.c (gfc_oacc_routine_dims): Replace with... + (gfc_oacc_routine_lop): ... this new function. + (gfc_match_oacc_routine): Adjust. + * trans-decl.c (add_attributes_to_decl): Likewise. + 2019-02-22 Thomas Schwinge <thomas@codesourcery.com> * openmp.c (gfc_match_oacc_declare): Revert earlier changes. diff --git a/gcc/fortran/gfortran.h b/gcc/fortran/gfortran.h index 6c4e839c489..f0258b39ffd 100644 --- a/gcc/fortran/gfortran.h +++ b/gcc/fortran/gfortran.h @@ -317,6 +317,15 @@ enum save_state { SAVE_NONE = 0, SAVE_EXPLICIT, SAVE_IMPLICIT }; +/* OpenACC 'routine' directive's level of parallelism. */ +enum oacc_routine_lop +{ OACC_ROUTINE_LOP_NONE = 0, + OACC_ROUTINE_LOP_GANG, + OACC_ROUTINE_LOP_WORKER, + OACC_ROUTINE_LOP_VECTOR, + OACC_ROUTINE_LOP_SEQ +}; + /* 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. */ @@ -904,8 +913,8 @@ typedef struct unsigned oacc_declare_device_resident:1; unsigned oacc_declare_link:1; - /* This is an OpenACC acclerator function at level N - 1 */ - unsigned oacc_function:3; + /* OpenACC 'routine' directive's level of parallelism. */ + ENUM_BITFIELD (oacc_routine_lop) oacc_routine_lop:3; /* Attributes set by compiler extensions (!GCC$ ATTRIBUTES). */ unsigned ext_attr:EXT_ATTR_NUM; diff --git a/gcc/fortran/openmp.c b/gcc/fortran/openmp.c index 8aa4a2f18c4..dfd4be86d50 100644 --- a/gcc/fortran/openmp.c +++ b/gcc/fortran/openmp.c @@ -2232,34 +2232,43 @@ gfc_match_oacc_cache (void) return MATCH_YES; } -/* Determine the loop level for a routine. */ +/* Determine the OpenACC 'routine' directive's level of parallelism. */ -static int -gfc_oacc_routine_dims (gfc_omp_clauses *clauses) +static oacc_routine_lop +gfc_oacc_routine_lop (gfc_omp_clauses *clauses) { - int level = -1; + oacc_routine_lop ret = OACC_ROUTINE_LOP_SEQ; if (clauses) { - unsigned mask = 0; + unsigned n_lop_clauses = 0; if (clauses->gang) - level = GOMP_DIM_GANG, mask |= GOMP_DIM_MASK (level); + { + ++n_lop_clauses; + ret = OACC_ROUTINE_LOP_GANG; + } if (clauses->worker) - level = GOMP_DIM_WORKER, mask |= GOMP_DIM_MASK (level); + { + ++n_lop_clauses; + ret = OACC_ROUTINE_LOP_WORKER; + } if (clauses->vector) - level = GOMP_DIM_VECTOR, mask |= GOMP_DIM_MASK (level); + { + ++n_lop_clauses; + ret = OACC_ROUTINE_LOP_VECTOR; + } if (clauses->seq) - level = GOMP_DIM_MAX, mask |= GOMP_DIM_MASK (level); + { + ++n_lop_clauses; + ret = OACC_ROUTINE_LOP_SEQ; + } - if (mask != (mask & -mask)) + if (n_lop_clauses > 1) gfc_error ("Multiple loop axes specified for routine"); } - if (level < 0) - level = GOMP_DIM_MAX; - - return level; + return ret; } match @@ -2352,8 +2361,8 @@ gfc_match_oacc_routine (void) gfc_current_ns->proc_name->name, &old_loc)) goto cleanup; - gfc_current_ns->proc_name->attr.oacc_function - = gfc_oacc_routine_dims (c) + 1; + gfc_current_ns->proc_name->attr.oacc_routine_lop + = gfc_oacc_routine_lop (c); } if (n) diff --git a/gcc/fortran/trans-decl.c b/gcc/fortran/trans-decl.c index 3604cfcf5cb..20d453051a2 100644 --- a/gcc/fortran/trans-decl.c +++ b/gcc/fortran/trans-decl.c @@ -46,6 +46,7 @@ along with GCC; see the file COPYING3. If not see #include "trans-stmt.h" #include "gomp-constants.h" #include "gimplify.h" +#include "omp-general.h" #define MAX_LABEL_VALUE 99999 @@ -1406,18 +1407,31 @@ add_attributes_to_decl (symbol_attribute sym_attr, tree list) list = tree_cons (get_identifier ("omp declare target"), NULL_TREE, list); - if (sym_attr.oacc_function) + if (sym_attr.oacc_routine_lop != OACC_ROUTINE_LOP_NONE) { - tree dims = NULL_TREE; - int ix; - int level = sym_attr.oacc_function - 1; - - for (ix = GOMP_DIM_MAX; ix--;) - dims = tree_cons (build_int_cst (boolean_type_node, ix >= level), - integer_zero_node, dims); + omp_clause_code code; + switch (sym_attr.oacc_routine_lop) + { + case OACC_ROUTINE_LOP_GANG: + code = OMP_CLAUSE_GANG; + break; + case OACC_ROUTINE_LOP_WORKER: + code = OMP_CLAUSE_WORKER; + break; + case OACC_ROUTINE_LOP_VECTOR: + code = OMP_CLAUSE_VECTOR; + break; + case OACC_ROUTINE_LOP_SEQ: + code = OMP_CLAUSE_SEQ; + break; + case OACC_ROUTINE_LOP_NONE: + default: + gcc_unreachable (); + } + tree c = build_omp_clause (UNKNOWN_LOCATION, code); - list = tree_cons (get_identifier ("oacc function"), - dims, list); + tree dims = oacc_build_routine_dims (c); + list = oacc_replace_fn_attrib_attr (list, dims); } return list; diff --git a/gcc/omp-general.c b/gcc/omp-general.c index 0f66ba0c5d8..356772ff458 100644 --- a/gcc/omp-general.c +++ b/gcc/omp-general.c @@ -540,16 +540,26 @@ oacc_launch_pack (unsigned code, tree device, unsigned op) /* Replace any existing oacc fn attribute with updated dimensions. */ -void -oacc_replace_fn_attrib (tree fn, tree dims) +/* Variant working on a list of attributes. */ + +tree +oacc_replace_fn_attrib_attr (tree attribs, tree dims) { tree ident = get_identifier (OACC_FN_ATTRIB); - tree attribs = DECL_ATTRIBUTES (fn); /* If we happen to be present as the first attrib, drop it. */ if (attribs && TREE_PURPOSE (attribs) == ident) attribs = TREE_CHAIN (attribs); - DECL_ATTRIBUTES (fn) = tree_cons (ident, dims, attribs); + return tree_cons (ident, dims, attribs); +} + +/* Variant working on a function decl. */ + +void +oacc_replace_fn_attrib (tree fn, tree dims) +{ + DECL_ATTRIBUTES (fn) + = oacc_replace_fn_attrib_attr (DECL_ATTRIBUTES (fn), dims); } /* Scan CLAUSES for launch dimensions and attach them to the oacc diff --git a/gcc/omp-general.h b/gcc/omp-general.h index 0cbbb31e73b..60faa5213a2 100644 --- a/gcc/omp-general.h +++ b/gcc/omp-general.h @@ -81,6 +81,7 @@ extern gimple *omp_build_barrier (tree lhs); extern poly_uint64 omp_max_vf (void); extern int omp_max_simt_vf (void); extern tree oacc_launch_pack (unsigned code, tree device, unsigned op); +extern tree oacc_replace_fn_attrib_attr (tree attribs, tree dims); extern void oacc_replace_fn_attrib (tree fn, tree dims); extern void oacc_set_fn_attrib (tree fn, tree clauses, vec<tree> *args); extern tree oacc_build_routine_dims (tree clauses); diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog index 12901a9361a..dec48441f30 100644 --- a/gcc/testsuite/ChangeLog +++ b/gcc/testsuite/ChangeLog @@ -1,5 +1,8 @@ 2019-02-22 Thomas Schwinge <thomas@codesourcery.com> + PR fortran/72741 + * gfortran.dg/goacc/classify-routine.f95: Adjust. + * c-c++-common/goacc/routine-5.c: Revert earlier changes. * g++.dg/goacc/template.C: Likewise. diff --git a/gcc/testsuite/gfortran.dg/goacc/classify-routine.f95 b/gcc/testsuite/gfortran.dg/goacc/classify-routine.f95 index 5cf4c13acb8..e435f5d7eae 100644 --- a/gcc/testsuite/gfortran.dg/goacc/classify-routine.f95 +++ b/gcc/testsuite/gfortran.dg/goacc/classify-routine.f95 @@ -21,10 +21,10 @@ subroutine ROUTINE end subroutine ROUTINE ! Check the offloaded function's attributes. -! { dg-final { scan-tree-dump-times "(?n)__attribute__\\(\\(omp declare target, oacc function \\(0 0, 1 0, 1 0\\)\\)\\)" 1 "ompexp" } } +! { dg-final { scan-tree-dump-times "(?n)__attribute__\\(\\(omp declare target, oacc function \\(0 1, 1 0, 1 0\\)\\)\\)" 1 "ompexp" } } ! Check the offloaded function's classification and compute dimensions (will ! always be 1 x 1 x 1 for non-offloading compilation). ! { dg-final { scan-tree-dump-times "(?n)Function is OpenACC routine level 1" 1 "oaccdevlow" } } ! { dg-final { scan-tree-dump-times "(?n)Compute dimensions \\\[1, 1, 1\\\]" 1 "oaccdevlow" } } -! { dg-final { scan-tree-dump-times "(?n)__attribute__\\(\\(oacc function \\(0 1, 1 1, 1 1\\), omp declare target, oacc function \\(0 0, 1 0, 1 0\\)\\)\\)" 1 "oaccdevlow" } } +! { dg-final { scan-tree-dump-times "(?n)__attribute__\\(\\(oacc function \\(0 1, 1 1, 1 1\\), omp declare target, oacc function \\(0 1, 1 0, 1 0\\)\\)\\)" 1 "oaccdevlow" } }
[nvptx] Unify C/Fortran routine handling in nvptx_goacc_validate_dims The Fortran front-end has a bug (PR72741) that means what when nvptx_goacc_validate_dims is called for a Fortran routine, the dims parameter is not the same as it would have been if the function would have been called for an equivalent C routine. Work around this bug by overriding the dims parameter for routines, allowing the function to handle routines in Fortran and C the same. Build and reg-tested on x86_64 with nvptx accelerator. 2018-12-17 Tom de Vries <tdevries@suse.de> * config/nvptx/nvptx.c (nvptx_goacc_validate_dims): Work around Fortran bug PR72741 by overriding dims parameter for routines. --- gcc/config/nvptx/nvptx.c | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/gcc/config/nvptx/nvptx.c b/gcc/config/nvptx/nvptx.c index 746d8bfb100..24727ad5920 100644 --- a/gcc/config/nvptx/nvptx.c +++ b/gcc/config/nvptx/nvptx.c @@ -5212,6 +5212,42 @@ nvptx_goacc_validate_dims (tree decl, int dims[], int fn_level) else gcc_unreachable (); + if (routine_p) + { + /* OpenACC routines in C arrive here with the following attributes + (omitting the 'omp declare target'): + seq : __attribute__((oacc function (0 1, 0 1, 0 1))) + vector: __attribute__((oacc function (0 1, 0 1, 1 0))) + worker: __attribute__((oacc function (0 1, 1 0, 1 0))) + gang : __attribute__((oacc function (1 0, 1 0, 1 0))) + + If we take f.i. the oacc function attribute of the worker routine + (0 1, 1 0, 1 0), then: + - the slice (0, 1, 1) is interpreted by oacc_fn_attrib_level as + meaning: worker routine, that is: + - can't contain gang loop (0), + - can contain worker loop (1), + - can contain vector loop (1). + - the slice (1, 0, 0) is interpreted by oacc_validate_dims as the + dimensions: gang: 1, worker: 0, vector: 0. + + OTOH, routines in Fortran arrive here with these attributes: + seq : __attribute__((oacc function (0 0, 0 0, 0 0))) + vector: __attribute__((oacc function (0 0, 0 0, 1 0))) + worker: __attribute__((oacc function (0 0, 1 0, 1 0))) + gang : __attribute__((oacc function (1 0, 1 0, 1 0))) + that is, the same as for C but with the dimensions set to 0. + + This is due to a bug in the Fortran front-end: PR72741. Work around + this bug by forcing the dimensions to be the same in Fortran as for C, + to be able to handle C and Fortran routines uniformly in this + function. */ + dims[GOMP_DIM_VECTOR] = fn_level > GOMP_DIM_VECTOR ? 1 : 0; + dims[GOMP_DIM_WORKER] = fn_level > GOMP_DIM_WORKER ? 1 : 0; + dims[GOMP_DIM_GANG] = fn_level > GOMP_DIM_GANG ? 1 : 0; + changed = true; + } + /* The vector size must be 32, unless this is a SEQ routine. */ if ((offload_region_p || oacc_default_dims_p || (routine_p && !routine_seq_p))