From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from esa4.mentor.iphmx.com (esa4.mentor.iphmx.com [68.232.137.252]) by sourceware.org (Postfix) with ESMTPS id AC10B3858D33; Tue, 28 Feb 2023 10:56:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org AC10B3858D33 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=codesourcery.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=mentor.com X-IronPort-AV: E=Sophos;i="5.98,221,1673942400"; d="scan'208";a="98452213" Received: from orw-gwy-01-in.mentorg.com ([192.94.38.165]) by esa4.mentor.iphmx.com with ESMTP; 28 Feb 2023 02:56:10 -0800 IronPort-SDR: tHuiHk+5KlT5cbzEQe/Rcne0YKP6k/6tVthWIY0FZiKykmHKq4G6WYHnvyrs9bfxSAc0737yfn tFEnxeM4ZBfM7bJp0K8o/cdo/tU6uC2xaf2BqfTZQPEfDcErubg7ta7iXZzmWT5wonMHXGkWXV hbfBfllkauZbjJy7d2qwxHDNUnCpV7HQ1U7PmNVDY/k4yrHmoqd6qzM1EEAYdq+2D1TWlr9PP9 L2MQNr3c51NiPmp+w0uFSTBBM5DT/E5wdc61P9FLdLeO8OFJHV5neM+i42sLaXW2jAOHQgP9pR ixc= From: Thomas Schwinge To: Tobias Burnus , Jakub Jelinek , Julian Brown CC: , Subject: Re: [Patch] OpenMP: Handle descriptors in target's firstprivate [PR104949] In-Reply-To: References: User-Agent: Notmuch/0.29.3+94~g74c3f1b (https://notmuchmail.org) Emacs/28.2 (x86_64-pc-linux-gnu) Date: Tue, 28 Feb 2023 11:56:01 +0100 Message-ID: <87o7pe12ke.fsf@euler.schwinge.homeip.net> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable X-Originating-IP: [137.202.0.90] X-ClientProxiedBy: svr-ies-mbx-15.mgc.mentorg.com (139.181.222.15) To svr-ies-mbx-10.mgc.mentorg.com (139.181.222.10) X-Spam-Status: No, score=-5.9 required=5.0 tests=BAYES_00,HEADER_FROM_DIFFERENT_DOMAINS,KAM_DMARC_STATUS,KAM_SHORT,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Hi! I'm currently reviewing 'gomp_copy_host2dev', 'ephemeral' in a different context, and a question came up here; commit r13-706-g49d1a2f91325fa8cc011149e27e5093a988b3a49 "OpenMP: Handle descriptors in target's firstprivate [PR104949]": On 2022-05-11T19:33:00+0200, Tobias Burnus wrote: > this patch handles (for target regions) > firstprivate(array_descriptor) > by not only firstprivatizing the descriptor but also the data > it points to. This is done by turning it in omp-low.cc the clause > into > firstprivate(descr) firstprivate(descr.data) > and then attaching the latter to the former. That works by > adding an 'attach' after the last firstprivate (and checking > for it in libgomp). The attached-to device address for a > previous (here: the first) firstprivate is obtained by returning > the device address inside the hostaddrs[i] alias omp_arr array, > i.e. the compiler generates: > omp_arr.1 =3D &descr; /* firstprivate */ > omp_arr.2 =3D descr.data; /* firstprivate */ > omp_arr.3 =3D &omp_arr.1; /* attach; bias: &desc.data-&desc */ > and libgomp then knows that the device address is in the > pointer. > Note: The code is not active for OpenACC. The existing code uses, e.g., > 'goto oacc_firstprivate' =E2=80=93 thus, the new code would be > partially active. I went for making it completely inactive for OpenACC > by adding one '!is_gimple_omp_oacc'. ACK. > I bet that a deep copy would be > also useful for OpenACC, but I have neither checked what the current > code does nor what the OpenACC spec says about this. Instead of adding corresponding handling to the OpenACC 'firstprivate' special code paths later on, I suggest that we first address known issues with OpenACC 'firstprivate' -- which probably may largely be achieved by in fact removing those 'goto oacc_firstprivate's and other special code paths? For example, see "OpenACC 'firstprivate' clause: initial value". That means, the following code currently isn't active for OpenACC, and given that OpenMP 'target' doesn't do asynchronous device execution (meaning: not in the way/implementation of OpenACC 'async'), it thus doesn't care about the 'ephemeral' argument to 'gomp_copy_host2dev', but still, for correctness (and once that code gets used for OpenACC): > OpenMP: Handle descriptors in target's firstprivate [PR104949] > > For allocatable/pointer arrays, a firstprivate to a device > not only needs to privatize the descriptor but also the actual > data. This is implemented as: > firstprivate(x) firstprivate(x.data) attach(x [bias: &x.data-&x) > where the address of x in device memory is saved in hostaddrs[i] > by libgomp and the middle end actually passes hostaddrs[i]' to > attach. > --- a/libgomp/target.c > +++ b/libgomp/target.c > @@ -1350,7 +1350,24 @@ gomp_map_vars_internal (struct gomp_device_descr *= devicep, > gomp_copy_host2dev (devicep, aq, > (void *) (tgt->tgt_start + tgt_size), > (void *) hostaddrs[i], len, false, cbuf= p); Here, passing 'ephemeral <- false' is correct, as 'h <- hostaddrs[i]' points to non-ephemeral data. > + /* Save device address in hostaddr to permit latter availab= lity > + when doing a deep-firstprivate with pointer attach. */ > + hostaddrs[i] =3D (void *) (tgt->tgt_start + tgt_size); Here, we modify 'hostaddrs[i]' (itself -- *not* the data that the original 'hostaddrs[i]' points to), so the above 'gomp_copy_host2dev' with 'ephemeral <- false' is still correct, right? > tgt_size +=3D len; > + > + /* If followed by GOMP_MAP_ATTACH, pointer assign this > + firstprivate to hostaddrs[i+1], which is assumed to cont= ain a > + device address. */ > + if (i + 1 < mapnum > + && (GOMP_MAP_ATTACH > + =3D=3D (typemask & get_kind (short_mapkind, kinds, = i+1)))) > + { > + uintptr_t target =3D (uintptr_t) hostaddrs[i]; > + void *devptr =3D *(void**) hostaddrs[i+1] + sizes[i+1]; > + gomp_copy_host2dev (devicep, aq, devptr, &target, > + sizeof (void *), false, cbufp); However, 'h <- &target' here points to data in the local frame ('target'), which potentially goes out of scope before an asynchronous 'gomp_copy_host2dev' has completed. Thus, don't we have to pass here 'ephemeral <- true' instead of 'ephemeral <- false'? Or, actually instead of '&target', pass '&hostaddrs[i]', which then again points to non-ephemeral data? Is the latter safe to do, or are we potentially further down the line modifying the data that '&hostaddrs[i]' points to? (I got a bit lost in the use of 'hostaddrs[i]' here.) > + ++i; > + } > continue; > case GOMP_MAP_FIRSTPRIVATE_INT: > case GOMP_MAP_ZERO_LEN_ARRAY_SECTION: > @@ -2517,6 +2534,11 @@ copy_firstprivate_data (char *tgt, size_t mapnum, = void **hostaddrs, > memcpy (tgt + tgt_size, hostaddrs[i], sizes[i]); > hostaddrs[i] =3D tgt + tgt_size; > tgt_size =3D tgt_size + sizes[i]; > + if (i + 1 < mapnum && (kinds[i+1] & 0xff) =3D=3D GOMP_MAP_ATTACH) > + { > + *(*(uintptr_t**) hostaddrs[i+1] + sizes[i+1]) =3D (uintptr_t) h= ostaddrs[i]; > + ++i; > + } > } > } For reference, the 'gomp_copy_host2dev' code that I highlighted above still triggers for the following test cases only: > --- /dev/null > +++ b/libgomp/testsuite/libgomp.fortran/target-firstprivate-1.f90 > @@ -0,0 +1,33 @@ > +! PR fortran/104949 > + > +implicit none (type,external) > +integer, allocatable :: A(:) > +A =3D [1,2,3,4,5,6] > + > +!$omp parallel firstprivate(A) > +!$omp master > + if (any (A /=3D [1,2,3,4,5])) error stop > + A(:) =3D [99,88,77,66,55] > +!$omp end master > +!$omp end parallel > + > +!$omp target firstprivate(A) > + if (any (A /=3D [1,2,3,4,5])) error stop > + A(:) =3D [99,88,77,66,55] > +!$omp end target > +if (any (A /=3D [1,2,3,4,5])) error stop > + > +!$omp parallel default(firstprivate) > +!$omp master > + if (any (A /=3D [1,2,3,4,5])) error stop > + A(:) =3D [99,88,77,66,55] > +!$omp end master > +!$omp end parallel > +if (any (A /=3D [1,2,3,4,5])) error stop > + > +!$omp target defaultmap(firstprivate) > + if (any (A /=3D [1,2,3,4,5])) error stop > + A(:) =3D [99,88,77,66,55] > +!$omp end target > +if (any (A /=3D [1,2,3,4,5])) error stop > +end > --- /dev/null > +++ b/libgomp/testsuite/libgomp.fortran/target-firstprivate-2.f90 > @@ -0,0 +1,113 @@ > +! PR fortran/104949 > + > +module m > +use omp_lib > +implicit none (type, external) > + > +contains > +subroutine one > + integer, allocatable :: x(:) > + integer :: i > + > + do i =3D 1, omp_get_num_devices() + 1 > + !$omp target firstprivate(x) > + if (allocated(x)) error stop > + !$omp end target > + if (allocated(x)) error stop > + end do > + > + do i =3D 1, omp_get_num_devices() + 1 > + !$omp target firstprivate(x, i) > + if (allocated(x)) error stop > + x =3D [10,20,30,40] + i > + if (any (x /=3D [10,20,30,40] + i)) error stop > + ! This leaks memory! > + ! deallocate(x) > + !$omp end target > + if (allocated(x)) error stop > + end do > + > + x =3D [1,2,3,4] > + > + do i =3D 1, omp_get_num_devices() + 1 > + !$omp target firstprivate(x, i) > + if (i <=3D 0) error stop > + if (.not.allocated(x)) error stop > + if (size(x) /=3D 4) error stop > + if (lbound(x,1) /=3D 1) error stop > + if (any (x /=3D [1,2,3,4])) error stop > + ! no reallocation, just malloced + assignment > + x =3D [10,20,30,40] + i > + if (any (x /=3D [10,20,30,40] + i)) error stop > + ! This leaks memory! > + ! deallocate(x) > + !$omp end target > + if (.not.allocated(x)) error stop > + if (size(x) /=3D 4) error stop > + if (lbound(x,1) /=3D 1) error stop > + if (any (x /=3D [1,2,3,4])) error stop > + end do > + deallocate(x) > +end > + > +subroutine two > + character(len=3D:), allocatable :: x(:) > + character(len=3D5) :: str > + integer :: i > + > + str =3D "abcde" ! work around for PR fortran/91544 > + do i =3D 1, omp_get_num_devices() + 1 > + !$omp target firstprivate(x) > + if (allocated(x)) error stop > + !$omp end target > + if (allocated(x)) error stop > + end do > + > + do i =3D 1, omp_get_num_devices() + 1 > + !$omp target firstprivate(x, i) > + if (allocated(x)) error stop > + ! no reallocation, just malloced + assignment > + x =3D [character(len=3D2+i) :: str,"fhji","klmno"] > + if (len(x) /=3D 2+i) error stop > + if (any (x /=3D [character(len=3D2+i) :: str,"fhji","klmno"])) err= or stop > + ! This leaks memory! > + ! deallocate(x) > + !$omp end target > + if (allocated(x)) error stop > + end do > + > + x =3D [character(len=3D4) :: "ABCDE","FHJI","KLMNO"] > + > + do i =3D 1, omp_get_num_devices() + 1 > + !$omp target firstprivate(x, i) > + if (i <=3D 0) error stop > + if (.not.allocated(x)) error stop > + if (size(x) /=3D 3) error stop > + if (lbound(x,1) /=3D 1) error stop > + if (len(x) /=3D 4) error stop > + if (any (x /=3D [character(len=3D4) :: "ABCDE","FHJI","KLMNO"])) e= rror stop > + !! Reallocation runs into the issue PR fortran/105538 > + !! > + !!x =3D [character(len=3D2+i) :: str,"fhji","klmno"] > + !!if (len(x) /=3D 2+i) error stop > + !!if (any (x /=3D [character(len=3D2+i) :: str,"fhji","klmno"])) e= rror stop > + !! This leaks memory! > + !! deallocate(x) > + ! Just assign: > + x =3D [character(len=3D4) :: "abcde","fhji","klmno"] > + if (any (x /=3D [character(len=3D4) :: "abcde","fhji","klmno"])) e= rror stop > + !$omp end target > + if (.not.allocated(x)) error stop > + if (lbound(x,1) /=3D 1) error stop > + if (size(x) /=3D 3) error stop > + if (len(x) /=3D 4) error stop > + if (any (x /=3D [character(len=3D4) :: "ABCDE","FHJI","KLMNO"])) err= or stop > + end do > + deallocate(x) > +end > +end module m > + > +use m > +call one > +call two > +end > --- /dev/null > +++ b/libgomp/testsuite/libgomp.fortran/target-firstprivate-3.f90 > @@ -0,0 +1,24 @@ > +implicit none > + integer, allocatable :: x(:) > + x =3D [1,2,3,4] > + call foo(x) > + if (any (x /=3D [1,2,3,4])) error stop > + call foo() > +contains > +subroutine foo(c) > + integer, allocatable, optional :: c(:) > + logical :: is_present > + is_present =3D present (c) > + !$omp target firstprivate(c) > + if (is_present) then > + if (.not. allocated(c)) error stop > + if (any (c /=3D [1,2,3,4])) error stop > + c =3D [99,88,77,66] > + if (any (c /=3D [99,88,77,66])) error stop > + end if > + !$omp end target > + if (is_present) then > + if (any (c /=3D [1,2,3,4])) error stop > + end if > +end > +end Gr=C3=BC=C3=9Fe Thomas ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstra=C3=9Fe 201= , 80634 M=C3=BCnchen; Gesellschaft mit beschr=C3=A4nkter Haftung; Gesch=C3= =A4ftsf=C3=BChrer: Thomas Heurung, Frank Th=C3=BCrauf; Sitz der Gesellschaf= t: M=C3=BCnchen; Registergericht M=C3=BCnchen, HRB 106955