From patchwork Fri Mar 24 16:18:08 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Thomas Schwinge X-Patchwork-Id: 66867 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 00F97384D18E for ; Fri, 24 Mar 2023 16:18:51 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org 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 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 X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" 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 firstprivate [PR104949]", see attached. Grüße Thomas > 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 = &descr; /* firstprivate */ >> omp_arr.2 = descr.data; /* firstprivate */ >> omp_arr.3 = &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' – 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, cbufp); > > Here, passing 'ephemeral <- false' is correct, as 'h <- hostaddrs[i]' > points to non-ephemeral data. > >> + /* Save device address in hostaddr to permit latter availablity >> + when doing a deep-firstprivate with pointer attach. */ >> + hostaddrs[i] = (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 += len; >> + >> + /* If followed by GOMP_MAP_ATTACH, pointer assign this >> + firstprivate to hostaddrs[i+1], which is assumed to contain a >> + device address. */ >> + if (i + 1 < mapnum >> + && (GOMP_MAP_ATTACH >> + == (typemask & get_kind (short_mapkind, kinds, i+1)))) >> + { >> + uintptr_t target = (uintptr_t) hostaddrs[i]; >> + void *devptr = *(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] = tgt + tgt_size; >> tgt_size = tgt_size + sizes[i]; >> + if (i + 1 < mapnum && (kinds[i+1] & 0xff) == GOMP_MAP_ATTACH) >> + { >> + *(*(uintptr_t**) hostaddrs[i+1] + sizes[i+1]) = (uintptr_t) hostaddrs[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 = [1,2,3,4,5,6] >> + >> +!$omp parallel firstprivate(A) >> +!$omp master >> + if (any (A /= [1,2,3,4,5])) error stop >> + A(:) = [99,88,77,66,55] >> +!$omp end master >> +!$omp end parallel >> + >> +!$omp target firstprivate(A) >> + if (any (A /= [1,2,3,4,5])) error stop >> + A(:) = [99,88,77,66,55] >> +!$omp end target >> +if (any (A /= [1,2,3,4,5])) error stop >> + >> +!$omp parallel default(firstprivate) >> +!$omp master >> + if (any (A /= [1,2,3,4,5])) error stop >> + A(:) = [99,88,77,66,55] >> +!$omp end master >> +!$omp end parallel >> +if (any (A /= [1,2,3,4,5])) error stop >> + >> +!$omp target defaultmap(firstprivate) >> + if (any (A /= [1,2,3,4,5])) error stop >> + A(:) = [99,88,77,66,55] >> +!$omp end target >> +if (any (A /= [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 = 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 = 1, omp_get_num_devices() + 1 >> + !$omp target firstprivate(x, i) >> + if (allocated(x)) error stop >> + x = [10,20,30,40] + i >> + if (any (x /= [10,20,30,40] + i)) error stop >> + ! This leaks memory! >> + ! deallocate(x) >> + !$omp end target >> + if (allocated(x)) error stop >> + end do >> + >> + x = [1,2,3,4] >> + >> + do i = 1, omp_get_num_devices() + 1 >> + !$omp target firstprivate(x, i) >> + if (i <= 0) error stop >> + if (.not.allocated(x)) error stop >> + if (size(x) /= 4) error stop >> + if (lbound(x,1) /= 1) error stop >> + if (any (x /= [1,2,3,4])) error stop >> + ! no reallocation, just malloced + assignment >> + x = [10,20,30,40] + i >> + if (any (x /= [10,20,30,40] + i)) error stop >> + ! This leaks memory! >> + ! deallocate(x) >> + !$omp end target >> + if (.not.allocated(x)) error stop >> + if (size(x) /= 4) error stop >> + if (lbound(x,1) /= 1) error stop >> + if (any (x /= [1,2,3,4])) error stop >> + end do >> + deallocate(x) >> +end >> + >> +subroutine two >> + character(len=:), allocatable :: x(:) >> + character(len=5) :: str >> + integer :: i >> + >> + str = "abcde" ! work around for PR fortran/91544 >> + do i = 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 = 1, omp_get_num_devices() + 1 >> + !$omp target firstprivate(x, i) >> + if (allocated(x)) error stop >> + ! no reallocation, just malloced + assignment >> + x = [character(len=2+i) :: str,"fhji","klmno"] >> + if (len(x) /= 2+i) error stop >> + if (any (x /= [character(len=2+i) :: str,"fhji","klmno"])) error stop >> + ! This leaks memory! >> + ! deallocate(x) >> + !$omp end target >> + if (allocated(x)) error stop >> + end do >> + >> + x = [character(len=4) :: "ABCDE","FHJI","KLMNO"] >> + >> + do i = 1, omp_get_num_devices() + 1 >> + !$omp target firstprivate(x, i) >> + if (i <= 0) error stop >> + if (.not.allocated(x)) error stop >> + if (size(x) /= 3) error stop >> + if (lbound(x,1) /= 1) error stop >> + if (len(x) /= 4) error stop >> + if (any (x /= [character(len=4) :: "ABCDE","FHJI","KLMNO"])) error stop >> + !! Reallocation runs into the issue PR fortran/105538 >> + !! >> + !!x = [character(len=2+i) :: str,"fhji","klmno"] >> + !!if (len(x) /= 2+i) error stop >> + !!if (any (x /= [character(len=2+i) :: str,"fhji","klmno"])) error stop >> + !! This leaks memory! >> + !! deallocate(x) >> + ! Just assign: >> + x = [character(len=4) :: "abcde","fhji","klmno"] >> + if (any (x /= [character(len=4) :: "abcde","fhji","klmno"])) error stop >> + !$omp end target >> + if (.not.allocated(x)) error stop >> + if (lbound(x,1) /= 1) error stop >> + if (size(x) /= 3) error stop >> + if (len(x) /= 4) error stop >> + if (any (x /= [character(len=4) :: "ABCDE","FHJI","KLMNO"])) error 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 = [1,2,3,4] >> + call foo(x) >> + if (any (x /= [1,2,3,4])) error stop >> + call foo() >> +contains >> +subroutine foo(c) >> + integer, allocatable, optional :: c(:) >> + logical :: is_present >> + is_present = present (c) >> + !$omp target firstprivate(c) >> + if (is_present) then >> + if (.not. allocated(c)) error stop >> + if (any (c /= [1,2,3,4])) error stop >> + c = [99,88,77,66] >> + if (any (c /= [99,88,77,66])) error stop >> + end if >> + !$omp end target >> + if (is_present) then >> + if (any (c /= [1,2,3,4])) error stop >> + end if >> +end >> +end > > > Grüße > Thomas ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955 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