| Message ID | 7f95349f1ef9bdd02ea7b70d15b74b1d50f00ab3.1612271845.git.julian@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
| Series | openacc: Mixing array elements and derived types | expand |
(added fortran@ at it is about Fortran) I note that some lines in the testcase are commented (polymorphic arrays), but I note further that Patch 3/4 enables them. On 02.02.21 14:28, Julian Brown wrote: > Derived-type members that are themselves derived types are not always > represented as pointers, so it is not always correct to dereference > them. The attached test case fails during gimplification without this > patch. > Tested with offloading to AMD GCN. OK for mainline? > > 2020-02-02 Julian Brown <julian@codesourcery.com> > > gcc/fortran/ > * trans-openmp.c (gfc_trans_omp_clauses): Handle non-pointer type. > > gcc/testsuite/ > * gfortran.dg/goacc/derived-classtypes-1.f95: New test. > --- > gcc/fortran/trans-openmp.c | 7 +- > .../goacc/derived-classtypes-1.f95 | 129 ++++++++++++++++++ > 2 files changed, 134 insertions(+), 2 deletions(-) > create mode 100644 gcc/testsuite/gfortran.dg/goacc/derived-classtypes-1.f95 > > diff --git a/gcc/fortran/trans-openmp.c b/gcc/fortran/trans-openmp.c > index 00358ca4d39..8d8da4593c3 100644 > --- a/gcc/fortran/trans-openmp.c > +++ b/gcc/fortran/trans-openmp.c > @@ -3013,7 +3013,8 @@ gfc_trans_omp_clauses (stmtblock_t *block, gfc_omp_clauses *clauses, > } > > OMP_CLAUSE_DECL (node) > - = build_fold_indirect_ref (data); > + = (POINTER_TYPE_P (TREE_TYPE (data)) > + ? build_fold_indirect_ref (data) : data); > OMP_CLAUSE_SIZE (node) = size; > node2 = build_omp_clause (input_location, > OMP_CLAUSE_MAP); I am a bit surprised given that: if (sym_attr.pointer || (openacc && sym_attr.allocatable)) I wonder whether we have a size problem: data = inner; size = TYPE_SIZE_UNIT (TREE_TYPE (inner)); Testing shows that it fails for: 80 | !$acc enter data copyin(jim%f) 95 | !$acc enter data copyin(pjim%f) 110 | !$acc enter data copyin(cjim%f) 125 | !$acc enter data copyin(acjim%f) where the component is 'type(aux), pointer :: f'. As sizeof(void*) and 2*sizeof(int) is the same, it does not matter in this case, but I fear it might in other cases. ('aux' contains two 'integer', one direct and one by inheriting it from 'aux1'.) Otherwise, I think the patch is fine. However, it also might be useful to add some character tests (kind=1 and kind=4) for completeness. NOTE: character(len=:), allocatable/pointer is the most interesting case, but that fails due to https://gcc.gnu.org/PR95868. Thus, we can only test for len=<const>. Tobias > @@ -3021,7 +3022,9 @@ gfc_trans_omp_clauses (stmtblock_t *block, gfc_omp_clauses *clauses, > openacc > ? GOMP_MAP_ATTACH_DETACH > : GOMP_MAP_ALWAYS_POINTER); > - OMP_CLAUSE_DECL (node2) = data; > + OMP_CLAUSE_DECL (node2) > + = (POINTER_TYPE_P (TREE_TYPE (data)) > + ? data : build_fold_addr_expr (data)); > OMP_CLAUSE_SIZE (node2) = size_int (0); > } > else > diff --git a/gcc/testsuite/gfortran.dg/goacc/derived-classtypes-1.f95 b/gcc/testsuite/gfortran.dg/goacc/derived-classtypes-1.f95 > new file mode 100644 > index 00000000000..e6cf09c6d3c > --- /dev/null > +++ b/gcc/testsuite/gfortran.dg/goacc/derived-classtypes-1.f95 > @@ -0,0 +1,129 @@ > +type :: type1 > + integer :: a > +end type type1 > + > +type :: type2 > + integer, pointer :: b > +end type type2 > + > +type :: aux1 > + integer :: y > +end type aux1 > + > +type, extends(aux1) :: aux > + integer :: x > +end type aux > + > +type :: type3 > + class(aux), pointer :: c(:) > +end type type3 > + > +type :: type4 > + integer, pointer :: d(:) > +end type type4 > + > +type :: type5 > + type(aux) :: e > +end type type5 > + > +type :: type6 > + type(aux), pointer :: f > +end type type6 > + > +type :: type7 > + class(aux), pointer :: g > +end type type7 > + > +type(type1) :: foo > +type(type2) :: bar > +type(type3) :: qux > +type(type4) :: quux > +type(type5) :: fred > +type(type6) :: jim > +type(type7) :: shiela > + > +type(type1), pointer :: pfoo > +type(type2), pointer :: pbar > +type(type3), pointer :: pqux > +type(type4), pointer :: pquux > +type(type5), pointer :: pfred > +type(type6), pointer :: pjim > +type(type7), pointer :: pshiela > + > +class(type1), pointer :: cfoo > +class(type2), pointer :: cbar > +class(type3), pointer :: cqux > +class(type4), pointer :: cquux > +class(type5), pointer :: cfred > +class(type6), pointer :: cjim > +class(type7), pointer :: cshiela > + > +class(type1), allocatable :: acfoo > +class(type2), allocatable :: acbar > +class(type3), allocatable :: acqux > +class(type4), allocatable :: acquux > +class(type5), allocatable :: acfred > +class(type6), allocatable :: acjim > +class(type7), allocatable :: acshiela > + > +!$acc enter data copyin(foo) > +!$acc enter data copyin(foo%a) > +!$acc enter data copyin(bar) > +!$acc enter data copyin(bar%b) > +!$acc enter data copyin(qux) > +!!$acc enter data copyin(qux%c) > +!$acc enter data copyin(quux) > +!$acc enter data copyin(quux%d) > +!$acc enter data copyin(fred) > +!$acc enter data copyin(fred%e) > +!$acc enter data copyin(jim) > +!$acc enter data copyin(jim%f) > +!$acc enter data copyin(shiela) > +!$acc enter data copyin(shiela%g) > + > +!$acc enter data copyin(pfoo) > +!$acc enter data copyin(pfoo%a) > +!$acc enter data copyin(pbar) > +!$acc enter data copyin(pbar%b) > +!$acc enter data copyin(pqux) > +!!$acc enter data copyin(pqux%c) > +!$acc enter data copyin(pquux) > +!$acc enter data copyin(pquux%d) > +!$acc enter data copyin(pfred) > +!$acc enter data copyin(pfred%e) > +!$acc enter data copyin(pjim) > +!$acc enter data copyin(pjim%f) > +!$acc enter data copyin(pshiela) > +!$acc enter data copyin(pshiela%g) > + > +!$acc enter data copyin(cfoo) > +!$acc enter data copyin(cfoo%a) > +!$acc enter data copyin(cbar) > +!$acc enter data copyin(cbar%b) > +!$acc enter data copyin(cqux) > +!!$acc enter data copyin(cqux%c) > +!$acc enter data copyin(cquux) > +!$acc enter data copyin(cquux%d) > +!$acc enter data copyin(cfred) > +!$acc enter data copyin(cfred%e) > +!$acc enter data copyin(cjim) > +!$acc enter data copyin(cjim%f) > +!$acc enter data copyin(cshiela) > +!$acc enter data copyin(cshiela%g) > + > +!$acc enter data copyin(acfoo) > +!$acc enter data copyin(acfoo%a) > +!$acc enter data copyin(acbar) > +!$acc enter data copyin(acbar%b) > +!$acc enter data copyin(acqux) > +!!$acc enter data copyin(acqux%c) > +!$acc enter data copyin(acquux) > +!$acc enter data copyin(acquux%d) > +!$acc enter data copyin(acfred) > +!$acc enter data copyin(acfred%e) > +!$acc enter data copyin(acjim) > +!$acc enter data copyin(acjim%f) > +!$acc enter data copyin(acshiela) > +!$acc enter data copyin(acshiela%g) > + > +end ----------------- Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank Thürauf
Hi Tobias, Thanks for review! Comments below. On Tue, 2 Feb 2021 17:32:03 +0100 Tobias Burnus <tobias@codesourcery.com> wrote: > > diff --git a/gcc/fortran/trans-openmp.c b/gcc/fortran/trans-openmp.c > > index 00358ca4d39..8d8da4593c3 100644 > > --- a/gcc/fortran/trans-openmp.c > > +++ b/gcc/fortran/trans-openmp.c > > @@ -3013,7 +3013,8 @@ gfc_trans_omp_clauses (stmtblock_t *block, > > gfc_omp_clauses *clauses, } > > > > OMP_CLAUSE_DECL (node) > > - = build_fold_indirect_ref (data); > > + = (POINTER_TYPE_P (TREE_TYPE (data)) > > + ? build_fold_indirect_ref (data) : > > data); OMP_CLAUSE_SIZE (node) = size; > > node2 = build_omp_clause > > (input_location, OMP_CLAUSE_MAP); > > I am a bit surprised given that: > if (sym_attr.pointer || (openacc && > sym_attr.allocatable)) > > I wonder whether we have a size problem: > data = inner; > size = TYPE_SIZE_UNIT (TREE_TYPE > (inner)); I think the code was correct wrt. data sizes, but the logic was admittedly rather convoluted. I think the attached is better -- the essential "weirdness" of the previous patch is that it seemed like a BT_DERIVED-typed "inner" could either be a pointer or not, and likewise for the BT_CLASS case. Actually though, that wasn't true. All the non-POINTER_TYPE_P cases involve BT_DERIVED, because the decl is transparently dereferenced by the earlier gfc_conv_component_ref call. BT_CLASS pointers, however, were not dereferenced -- so "data" appeared as an actual pointer. The pre-patch code actually only worked in the BT_CLASS case. So, I think the new version makes more sense. (The "size" field is either transparently dereferenced via gfc_conv_component_ref, or comes from the class's vtable, so it's never the size of the non-dereferenced pointer.) Re-tested with offloading to AMD GCN. OK? Thank you, Julian
Hi Julian,
LGTM. Thanks!
* * *
Can you as follow-up also add a testcase which uses – instead of integer –
'character' (with len > 1, both kind=1 and kind=4) and check that it is
handled correctly? In particular, that the right size is passed on.
(Should be size-in-bytes = kind*len.) It probably just work, unless any
OpenACC construct uses a different code path than the one covered by my
OpenMP patches (esp. https://gcc.gnu.org/g:102502e32ea4e8a75d6b252ba319d09d735d9aa7 ).
('character' (esp. kind=4 or len=: (¹)) and 'class' have the tendency to
break. Not that allocatable/pointer/optional don't expose bugs.)
Tobias
(¹) Bug regarding (len=:) in components ishttps://gcc.gnu.org/PR95868.
On 04.02.21 14:58, Julian Brown wrote:
> Thanks for review! Comments below.
> [...]
> I think the code was correct wrt. data sizes, but the logic was
> admittedly rather convoluted. I think the attached is better -- the
> essential "weirdness" of the previous patch is that it seemed like a
> BT_DERIVED-typed "inner" could either be a pointer or not, and likewise
> for the BT_CLASS case.
>
> Actually though, that wasn't true. All the non-POINTER_TYPE_P cases
> involve BT_DERIVED, because the decl is transparently dereferenced
> by the earlier gfc_conv_component_ref call. BT_CLASS pointers, however,
> were not dereferenced -- so "data" appeared as an actual pointer. The
> pre-patch code actually only worked in the BT_CLASS case.
>
> So, I think the new version makes more sense. (The "size" field is
> either transparently dereferenced via gfc_conv_component_ref, or comes
> from the class's vtable, so it's never the size of the non-dereferenced
> pointer.)
>
> Re-tested with offloading to AMD GCN. OK?
>
> Thank you,
>
> Julian
-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank Thürauf
On Thu, 4 Feb 2021 15:55:20 +0100 Tobias Burnus <tobias@codesourcery.com> wrote: > Hi Julian, > > LGTM. Thanks! > > * * * > > Can you as follow-up also add a testcase which uses – instead of > integer – 'character' (with len > 1, both kind=1 and kind=4) and > check that it is handled correctly? In particular, that the right > size is passed on. (Should be size-in-bytes = kind*len.) It probably > just work, unless any OpenACC construct uses a different code path > than the one covered by my OpenMP patches (esp. > https://gcc.gnu.org/g:102502e32ea4e8a75d6b252ba319d09d735d9aa7 ). > > ('character' (esp. kind=4 or len=: (¹)) and 'class' have the tendency > to break. Not that allocatable/pointer/optional don't expose bugs.) Thanks, I've committed the patch, and also the attached patch with some additional character tests. All seems good, AFAICT. Julian
diff --git a/gcc/fortran/trans-openmp.c b/gcc/fortran/trans-openmp.c index 00358ca4d39..8d8da4593c3 100644 --- a/gcc/fortran/trans-openmp.c +++ b/gcc/fortran/trans-openmp.c @@ -3013,7 +3013,8 @@ gfc_trans_omp_clauses (stmtblock_t *block, gfc_omp_clauses *clauses, } OMP_CLAUSE_DECL (node) - = build_fold_indirect_ref (data); + = (POINTER_TYPE_P (TREE_TYPE (data)) + ? build_fold_indirect_ref (data) : data); OMP_CLAUSE_SIZE (node) = size; node2 = build_omp_clause (input_location, OMP_CLAUSE_MAP); @@ -3021,7 +3022,9 @@ gfc_trans_omp_clauses (stmtblock_t *block, gfc_omp_clauses *clauses, openacc ? GOMP_MAP_ATTACH_DETACH : GOMP_MAP_ALWAYS_POINTER); - OMP_CLAUSE_DECL (node2) = data; + OMP_CLAUSE_DECL (node2) + = (POINTER_TYPE_P (TREE_TYPE (data)) + ? data : build_fold_addr_expr (data)); OMP_CLAUSE_SIZE (node2) = size_int (0); } else diff --git a/gcc/testsuite/gfortran.dg/goacc/derived-classtypes-1.f95 b/gcc/testsuite/gfortran.dg/goacc/derived-classtypes-1.f95 new file mode 100644 index 00000000000..e6cf09c6d3c --- /dev/null +++ b/gcc/testsuite/gfortran.dg/goacc/derived-classtypes-1.f95 @@ -0,0 +1,129 @@ +type :: type1 + integer :: a +end type type1 + +type :: type2 + integer, pointer :: b +end type type2 + +type :: aux1 + integer :: y +end type aux1 + +type, extends(aux1) :: aux + integer :: x +end type aux + +type :: type3 + class(aux), pointer :: c(:) +end type type3 + +type :: type4 + integer, pointer :: d(:) +end type type4 + +type :: type5 + type(aux) :: e +end type type5 + +type :: type6 + type(aux), pointer :: f +end type type6 + +type :: type7 + class(aux), pointer :: g +end type type7 + +type(type1) :: foo +type(type2) :: bar +type(type3) :: qux +type(type4) :: quux +type(type5) :: fred +type(type6) :: jim +type(type7) :: shiela + +type(type1), pointer :: pfoo +type(type2), pointer :: pbar +type(type3), pointer :: pqux +type(type4), pointer :: pquux +type(type5), pointer :: pfred +type(type6), pointer :: pjim +type(type7), pointer :: pshiela + +class(type1), pointer :: cfoo +class(type2), pointer :: cbar +class(type3), pointer :: cqux +class(type4), pointer :: cquux +class(type5), pointer :: cfred +class(type6), pointer :: cjim +class(type7), pointer :: cshiela + +class(type1), allocatable :: acfoo +class(type2), allocatable :: acbar +class(type3), allocatable :: acqux +class(type4), allocatable :: acquux +class(type5), allocatable :: acfred +class(type6), allocatable :: acjim +class(type7), allocatable :: acshiela + +!$acc enter data copyin(foo) +!$acc enter data copyin(foo%a) +!$acc enter data copyin(bar) +!$acc enter data copyin(bar%b) +!$acc enter data copyin(qux) +!!$acc enter data copyin(qux%c) +!$acc enter data copyin(quux) +!$acc enter data copyin(quux%d) +!$acc enter data copyin(fred) +!$acc enter data copyin(fred%e) +!$acc enter data copyin(jim) +!$acc enter data copyin(jim%f) +!$acc enter data copyin(shiela) +!$acc enter data copyin(shiela%g) + +!$acc enter data copyin(pfoo) +!$acc enter data copyin(pfoo%a) +!$acc enter data copyin(pbar) +!$acc enter data copyin(pbar%b) +!$acc enter data copyin(pqux) +!!$acc enter data copyin(pqux%c) +!$acc enter data copyin(pquux) +!$acc enter data copyin(pquux%d) +!$acc enter data copyin(pfred) +!$acc enter data copyin(pfred%e) +!$acc enter data copyin(pjim) +!$acc enter data copyin(pjim%f) +!$acc enter data copyin(pshiela) +!$acc enter data copyin(pshiela%g) + +!$acc enter data copyin(cfoo) +!$acc enter data copyin(cfoo%a) +!$acc enter data copyin(cbar) +!$acc enter data copyin(cbar%b) +!$acc enter data copyin(cqux) +!!$acc enter data copyin(cqux%c) +!$acc enter data copyin(cquux) +!$acc enter data copyin(cquux%d) +!$acc enter data copyin(cfred) +!$acc enter data copyin(cfred%e) +!$acc enter data copyin(cjim) +!$acc enter data copyin(cjim%f) +!$acc enter data copyin(cshiela) +!$acc enter data copyin(cshiela%g) + +!$acc enter data copyin(acfoo) +!$acc enter data copyin(acfoo%a) +!$acc enter data copyin(acbar) +!$acc enter data copyin(acbar%b) +!$acc enter data copyin(acqux) +!!$acc enter data copyin(acqux%c) +!$acc enter data copyin(acquux) +!$acc enter data copyin(acquux%d) +!$acc enter data copyin(acfred) +!$acc enter data copyin(acfred%e) +!$acc enter data copyin(acjim) +!$acc enter data copyin(acjim%f) +!$acc enter data copyin(acshiela) +!$acc enter data copyin(acshiela%g) + +end