From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 92448 invoked by alias); 7 Dec 2015 15:55:50 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 92427 invoked by uid 89); 7 Dec 2015 15:55:50 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-Spam-User: qpsmtpd, 2 recipients X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 07 Dec 2015 15:55:49 +0000 Received: from svr-orw-fem-05.mgc.mentorg.com ([147.34.97.43]) by relay1.mentorg.com with esmtp id 1a5y8f-00002u-N0 from Cesar_Philippidis@mentor.com ; Mon, 07 Dec 2015 07:55:45 -0800 Received: from [127.0.0.1] (147.34.91.1) by svr-orw-fem-05.mgc.mentorg.com (147.34.97.43) with Microsoft SMTP Server id 14.3.224.2; Mon, 7 Dec 2015 07:55:45 -0800 Subject: Re: [gomp4] Fix Fortran deviceptr To: James Norris , GCC Patches References: <56644BBC.1050602@codesourcery.com> CC: , Thomas Schwinge From: Cesar Philippidis Message-ID: <5665AC01.2020008@codesourcery.com> Date: Mon, 07 Dec 2015 15:55:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <56644BBC.1050602@codesourcery.com> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-SW-Source: 2015-12/txt/msg00755.txt.bz2 On 12/06/2015 06:52 AM, James Norris wrote: > This patch fixes a some runtime issues when dealing with > the deviceptr clause in Fortran. There were some corner > cases that were not being dealt with correctly, and the > patch resolves these. Also a new set of test cases has > been added. Which corner cases? > diff --git a/libgomp/ChangeLog.gomp b/libgomp/ChangeLog.gomp > index a2f1c31..791aa4c 100644 > --- a/libgomp/ChangeLog.gomp > +++ b/libgomp/ChangeLog.gomp > @@ -1,3 +1,10 @@ > +2015-12-06 James Norris > + > + * oacc-parallel.c (GOACC_parallel_keyed, GOACC_data_start): > + Handle Fortran deviceptr clause combination. > + * testsuite/libgomp.oacc-fortran/deviceptr-1.f90: New test. > + * testsuite/libgomp.oacc-fortran/declare-1.f90: Remove erroneous test. > + > 2015-12-05 Chung-Lin Tang > > * oacc-plugin.h (GOMP_PLUGIN_async_unmap_vars): Add int parameter. > diff --git a/libgomp/oacc-parallel.c b/libgomp/oacc-parallel.c > index a4b2c01..a606152 100644 > --- a/libgomp/oacc-parallel.c > +++ b/libgomp/oacc-parallel.c > @@ -99,18 +99,37 @@ GOACC_parallel_keyed (int device, void (*fn) (void *), > thr = goacc_thread (); > acc_dev = thr->dev; > > - for (i = 0; i < (signed)(mapnum - 1); i++) > + for (i = 0; i < mapnum; i++) > { > unsigned short kind1 = kinds[i] & 0xff; > - unsigned short kind2 = kinds[i+1] & 0xff; > > /* Handle Fortran deviceptr clause. */ > - if ((kind1 == GOMP_MAP_FORCE_DEVICEPTR && kind2 == GOMP_MAP_POINTER) > - && (sizes[i + 1] == 0) > - && (hostaddrs[i] == *(void **)hostaddrs[i + 1])) > + if (kind1 == GOMP_MAP_FORCE_DEVICEPTR) > { > - kinds[i+1] = kinds[i]; > - sizes[i+1] = sizeof (void *); > + unsigned short kind2; > + > + if (i < (signed)mapnum - 1) > + kind2 = kinds[i + 1] & 0xff; > + else > + kind2 = 0xffff; > + > + if (sizes[i] == sizeof (void *)) > + continue; > + > + /* At this point, we're dealing with a Fortran deviceptr. > + If the next element is not what we're expecting, then > + this is an instance of where the deviceptr variable was > + not used within the region and the pointer was removed > + by the gimplifier. */ > + if (kind2 == GOMP_MAP_POINTER > + && sizes[i + 1] == 0 > + && hostaddrs[i] == *(void **)hostaddrs[i + 1]) > + { > + kinds[i+1] = kinds[i]; > + sizes[i+1] = sizeof (void *); > + } > + > + /* Invalidate the entry. */ > hostaddrs[i] = NULL; > } > } > @@ -254,18 +273,38 @@ GOACC_data_start (int device, size_t mapnum, > struct goacc_thread *thr = goacc_thread (); > struct gomp_device_descr *acc_dev = thr->dev; > > - for (i = 0; i < (signed)(mapnum - 1); i++) > + for (i = 0; i < mapnum; i++) > { > unsigned short kind1 = kinds[i] & 0xff; > - unsigned short kind2 = kinds[i+1] & 0xff; > > /* Handle Fortran deviceptr clause. */ > - if ((kind1 == GOMP_MAP_FORCE_DEVICEPTR && kind2 == GOMP_MAP_POINTER) > - && (sizes[i + 1] == 0) > - && (hostaddrs[i] == *(void **)hostaddrs[i + 1])) > + if (kind1 == GOMP_MAP_FORCE_DEVICEPTR) > { > - kinds[i+1] = kinds[i]; > - sizes[i+1] = sizeof (void *); > + unsigned short kind2; > + > + if (i < (signed)mapnum - 1) > + kind2 = kinds[i + 1] & 0xff; > + else > + kind2 = 0xffff; > + > + /* If the size is right, skip it. */ > + if (sizes[i] == sizeof (void *)) > + continue; > + > + /* At this point, we're dealing with a Fortran deviceptr. > + If the next element is not what we're expecting, then > + this is an instance of where the deviceptr variable was > + not used within the region and the pointer was removed > + by the gimplifier. */ > + if (kind2 == GOMP_MAP_POINTER > + && sizes[i + 1] == 0 > + && hostaddrs[i] == *(void **)hostaddrs[i + 1]) > + { > + kinds[i+1] = kinds[i]; > + sizes[i+1] = sizeof (void *); > + } > + > + /* Invalidate the entry. */ > hostaddrs[i] = NULL; > } > } Two observations: 1. Why is deviceptr so special that gomp_map_vars can't handle it directly? 2. It appears that deviceptr code in GOACC_parallel_keyed is mostly identical to GOACC_data_start. Can you put that duplicate code into a function? That would be easier to maintain in the long run. > diff --git a/libgomp/testsuite/libgomp.oacc-fortran/declare-1.f90 b/libgomp/testsuite/libgomp.oacc-fortran/declare-1.f90 > index 430cd24..e781878 100644 > --- a/libgomp/testsuite/libgomp.oacc-fortran/declare-1.f90 > +++ b/libgomp/testsuite/libgomp.oacc-fortran/declare-1.f90 > @@ -1,6 +1,4 @@ > ! { dg-do run { target openacc_nvidia_accel_selected } } > -! libgomp: cuStreamSynchronize error: an illegal memory access was encountered > -! { dg-xfail-run-if "TODO" { *-*-* } } Maybe add a comment to describe what the test is doing like you did ... > diff --git a/libgomp/testsuite/libgomp.oacc-fortran/deviceptr-1.f90 b/libgomp/testsuite/libgomp.oacc-fortran/deviceptr-1.f90 > new file mode 100644 > index 0000000..879cbf1 > --- /dev/null > +++ b/libgomp/testsuite/libgomp.oacc-fortran/deviceptr-1.f90 > @@ -0,0 +1,197 @@ > +! { dg-do run } > + > +!! Test the deviceptr clause with various directives > +!! and in combination with other directives where > +!! the deviceptr variable is implied. ... here. And minor nit, but the other fortran tests only use a single exclamation point for comments. This test should also use a single one for consistency. Cesar