From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from esa2.mentor.iphmx.com (esa2.mentor.iphmx.com [68.232.141.98]) by sourceware.org (Postfix) with ESMTPS id 067A33858D1E; Fri, 24 Mar 2023 16:18:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 067A33858D1E 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,288,1673942400"; d="scan'208,223";a="320855" Received: from orw-gwy-02-in.mentorg.com ([192.94.38.167]) by esa2.mentor.iphmx.com with ESMTP; 24 Mar 2023 08:18:19 -0800 IronPort-SDR: jVYDMbeKXXLXMLxZXVpVNNMa1QeJKjCup/4iTk5ageAWKdyb7gFd1NFpJ4BSElGrbWOnu2YPdu jIJBC7r86mSuV+DwcT6EiT8TbEtiS0brN7esWzIbPNqxlCBBjPoXCXc8qyNqWg4svP05RROI/j ENXfxge+GjCN+Vzg70A+aAjuvRxxiPNNbZT44APFEbMXkh+vBDUcbzafJ71Y8b1lJBolkhUfzk 55K4EuBPJV9Ac9xRqtL5RRf4hT/0dJ9AQaMUk2jZTRLJV4PzMbGPipIZ8Al9IZP1NasArIh18o 8D0= From: Thomas Schwinge To: , Tobias Burnus , "Jakub Jelinek" , Julian Brown CC: Subject: Add caveat/safeguard to OpenMP: Handle descriptors in target's firstprivate [PR104949] (was: [Patch] OpenMP: Handle descriptors in target's firstprivate [PR104949]) In-Reply-To: <87o7pe12ke.fsf@euler.schwinge.homeip.net> References: <87o7pe12ke.fsf@euler.schwinge.homeip.net> User-Agent: Notmuch/0.29.3+94~g74c3f1b (https://notmuchmail.org) Emacs/28.2 (x86_64-pc-linux-gnu) Date: Fri, 24 Mar 2023 17:18:08 +0100 Message-ID: <878rfm9l8f.fsf@euler.schwinge.homeip.net> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" X-Originating-IP: [137.202.0.90] X-ClientProxiedBy: svr-ies-mbx-13.mgc.mentorg.com (139.181.222.13) To svr-ies-mbx-10.mgc.mentorg.com (139.181.222.10) X-Spam-Status: No, score=-11.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,HEADER_FROM_DIFFERENT_DOMAINS,KAM_DMARC_STATUS,KAM_SHORT,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: --=-=-= Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi! On 2023-02-28T11:56:01+0100, I wrote: > 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]": It doesn't seem as if we're going to address this question anytime soon, but it also isn't necessary; the worrysome condition currently cannot arise. I've therefore now documented that via 'assert (!aq)', and pushed to master branch commit e8fec6998b656dac02d4bc6c69b35a0fb5611e87 "Add caveat/safeguard to OpenMP: Handle descriptors in target's firstprivat= e [PR104949]", see attached. Gr=C3=BC=C3=9Fe Thomas > On 2022-05-11T19:33:00+0200, Tobias Burnus wrot= e: >> 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"])) er= ror 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"])) = error 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"])) = error 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"])) = error 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"])) er= ror 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 --=-=-= Content-Type: text/x-diff Content-Disposition: inline; filename="0001-Add-caveat-safeguard-to-OpenMP-Handle-descriptors-in.patch" >From e8fec6998b656dac02d4bc6c69b35a0fb5611e87 Mon Sep 17 00:00:00 2001 From: Thomas Schwinge Date: Thu, 23 Mar 2023 12:32:35 +0100 Subject: [PATCH] Add caveat/safeguard to OpenMP: Handle descriptors in target's firstprivate [PR104949] Follow-up to commit 49d1a2f91325fa8cc011149e27e5093a988b3a49 "OpenMP: Handle descriptors in target's firstprivate [PR104949]". PR fortran/104949 libgomp/ * target.c (gomp_map_vars_internal) : Add caveat/safeguard. --- libgomp/target.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/libgomp/target.c b/libgomp/target.c index 90b4204133a..b30c6a50c7e 100644 --- a/libgomp/target.c +++ b/libgomp/target.c @@ -1396,6 +1396,11 @@ gomp_map_vars_internal (struct gomp_device_descr *devicep, { uintptr_t target = (uintptr_t) hostaddrs[i]; void *devptr = *(void**) hostaddrs[i+1] + sizes[i+1]; + /* Per + + "OpenMP: Handle descriptors in target's firstprivate [PR104949]" + this probably needs revision for 'aq' usage. */ + assert (!aq); gomp_copy_host2dev (devicep, aq, devptr, &target, sizeof (void *), false, cbufp); ++i; -- 2.25.1 --=-=-=--