On Fri, 5 Feb 2021 17:25:10 +0100
Tobias Burnus <[email protected]> wrote:
> (CC fortran@)
>
> Hi Julian,
>
> not doing an extensive review yet, but the following gives an ICE
> with this patch applied. (I believe the others are already in, aren't
> they?)
>
> type t
> integer :: i, j
> end type t
> type t2
> type(t) :: b(4)
> end type
> type(t2) :: var(10)
> !$acc update host(var(3)%b(:)%j)
> !$acc update host(var(3)%b%j)
> end
>
> That's a noncontiguous array – which is permitted for 'update'
> and it gives an ICE via:
>
> 0x9b0c59 gfc_conv_scalarized_array_ref
> ../../repos/gcc/gcc/fortran/trans-array.c:3570
> 0x9b2134 gfc_conv_array_ref(gfc_se*, gfc_array_ref*, gfc_expr*,
> locus*) ../../repos/gcc/gcc/fortran/trans-array.c:3721
> 0x9e9cc6 gfc_conv_variable
> ../../repos/gcc/gcc/fortran/trans-expr.c:2998
> 0xa22682 gfc_trans_omp_clauses
> ../../repos/gcc/gcc/fortran/trans-openmp.c:2963
I think the attached patch fixes that. (This could be merged into the
parent patch or kept separate, not sure which is better.)
Re-tested with offloading to AMD GCN. OK?
> > + bool allocatable = false, pointer = false;
> > +
> > + if (lastref && lastref->type == REF_COMPONENT)
> > + {
> > + gfc_component *c = lastref->u.c.component;
> > +
> > + if (c->ts.type == BT_CLASS)
> > + {
> > + pointer = CLASS_DATA (c)->attr.class_pointer;
> > + allocatable = CLASS_DATA
> > (c)->attr.allocatable;
> > + }
> > + else
> > + {
> > + pointer = c->attr.pointer;
> > + allocatable = c->attr.allocatable;
> > + }
> > + }
> > +
>
> I am not sure how the rest will change, but I was wondering
> whether the following helps. I see that 'lastref' is used
> elsewhere – hence, I am not sure whether it is indeed better.
>
> symbol_attribute attr = {};
> if (n->expr)
> attr = gfc_expr_attr (n->expr);
Ah, I didn't know about that one! But yeah, not sure if it's better
here.
Thanks for (pre-)review!
Julian
>From adf4221bf5b5ab01ce1ed264226f1799d8aa0b05 Mon Sep 17 00:00:00 2001
From: Julian Brown <[email protected]>
Date: Sat, 6 Feb 2021 02:34:30 -0800
Subject: [PATCH] Handle discontinuous ranges with derived types
OpenACC "update" allows discontiguous ranges to be specified, e.g. by
selecting an array slice in the middle of a derived type selector list:
!$acc update host(mytype%arraymember(:)%foo)
We handle this by transferring the whole of the slice and ignoring the
"foo" part.
gcc/fortran/
* trans-openmp.c (gfc_trans_omp_clauses): Handle discontiguous
ranges specified by arrays or array slices with trailing
derived-type selectors.
gcc/testsuite/
* gfortran.dg/goacc/array-with-dt-6.f90: New test.
libgomp/
* testsuite/gfortran.dg/update-dt-array-2.f90: New test.
---
gcc/fortran/trans-openmp.c | 26 +++++++-
.../gfortran.dg/goacc/array-with-dt-6.f90 | 10 ++++
.../update-dt-array-2.f90 | 59 +++++++++++++++++++
3 files changed, 93 insertions(+), 2 deletions(-)
create mode 100644 gcc/testsuite/gfortran.dg/goacc/array-with-dt-6.f90
create mode 100644 libgomp/testsuite/libgomp.oacc-fortran/update-dt-array-2.f90
diff --git a/gcc/fortran/trans-openmp.c b/gcc/fortran/trans-openmp.c
index 67e370f8b57..758485e0c7d 100644
--- a/gcc/fortran/trans-openmp.c
+++ b/gcc/fortran/trans-openmp.c
@@ -2676,12 +2676,32 @@ gfc_trans_omp_clauses (stmtblock_t *block, gfc_omp_clauses *clauses,
if (DECL_P (decl))
TREE_ADDRESSABLE (decl) = 1;
- gfc_ref *lastref = NULL;
+ gfc_ref *lastref = NULL, *lastslice = NULL;
if (n->expr)
for (gfc_ref *ref = n->expr->ref; ref; ref = ref->next)
- if (ref->type == REF_COMPONENT || ref->type == REF_ARRAY)
+ if (ref->type == REF_COMPONENT)
lastref = ref;
+ else if (ref->type == REF_ARRAY)
+ {
+ if (ref->u.ar.type == AR_FULL
+ || ref->u.ar.type == AR_SECTION)
+ lastslice = ref;
+
+ lastref = ref;
+ }
+
+ /* If a slice is specified but it is not the last ref, this
+ might be an update operation that allows discontiguous
+ regions, like:
+
+ myvar%slice(:)%foo
+
+ in that case, we will ignore the %foo part and transfer the
+ whole of the slice as a single block. */
+
+ if (lastslice)
+ lastref = lastslice;
bool allocatable = false, pointer = false;
@@ -3023,6 +3043,8 @@ gfc_trans_omp_clauses (stmtblock_t *block, gfc_omp_clauses *clauses,
if (ref->u.ar.type == AR_ELEMENT && ref->next)
gfc_conv_array_ref (&se, &ref->u.ar, n->expr,
&n->expr->where);
+ else if (ref == lastslice)
+ break;
else
gcc_assert (!ref->next);
}
diff --git a/gcc/testsuite/gfortran.dg/goacc/array-with-dt-6.f90 b/gcc/testsuite/gfortran.dg/goacc/array-with-dt-6.f90
new file mode 100644
index 00000000000..260db6602f4
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/goacc/array-with-dt-6.f90
@@ -0,0 +1,10 @@
+type t
+ integer :: i, j
+end type t
+type t2
+ type(t) :: b(4)
+end type
+type(t2) :: var(10)
+!$acc update host(var(3)%b(:)%j)
+!$acc update host(var(3)%b%j)
+end
diff --git a/libgomp/testsuite/libgomp.oacc-fortran/update-dt-array-2.f90 b/libgomp/testsuite/libgomp.oacc-fortran/update-dt-array-2.f90
new file mode 100644
index 00000000000..8e0ed338d39
--- /dev/null
+++ b/libgomp/testsuite/libgomp.oacc-fortran/update-dt-array-2.f90
@@ -0,0 +1,59 @@
+! { dg-do run }
+
+program myprog
+
+ type mt2
+ integer :: p, q, r
+ end type mt2
+
+ type mytype
+ type(mt2), allocatable :: myarr(:,:)
+ end type mytype
+ integer :: i
+
+ type(mytype), allocatable :: typearr(:)
+
+ allocate(typearr(1:100))
+
+ do i=1,100
+ allocate(typearr(i)%myarr(1:100,1:100))
+ end do
+
+ do i=1,100
+ typearr(i)%myarr(:,:)%p = 0
+ typearr(i)%myarr(:,:)%q = 0
+ typearr(i)%myarr(:,:)%r = 0
+ end do
+
+ !$acc enter data copyin(typearr)
+
+ do i=1,100
+ !$acc enter data copyin(typearr(i)%myarr)
+ end do
+
+ i=33
+ typearr(i)%myarr(:,:)%q = 50
+
+ !$acc update device(typearr(i)%myarr(:,:)%q)
+
+ do i=1,100
+ !$acc exit data copyout(typearr(i)%myarr)
+ end do
+
+ !$acc exit data delete(typearr)
+
+ do i=1,100
+ if (i.eq.33) then
+ if (any(typearr(i)%myarr(:,:)%q.ne.50)) stop 1
+ else
+ if (any(typearr(i)%myarr(:,:)%q.ne.0)) stop 2
+ end if
+ end do
+
+ do i=1,100
+ deallocate(typearr(i)%myarr)
+ end do
+
+ deallocate(typearr)
+
+end program myprog
--
2.29.2
