From patchwork Tue Jun 18 23:06:19 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Thomas Schwinge X-Patchwork-Id: 1118381 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=gcc.gnu.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=gcc-patches-return-503232-incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=codesourcery.com Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="ES5aO2RA"; dkim-atps=neutral Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 45T3cd5BYcz9s4Y for ; Wed, 19 Jun 2019 09:06:57 +1000 (AEST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:in-reply-to:references:date:message-id :mime-version:content-type; q=dns; s=default; b=ZP8Pap8F2w1SRFLu 5ebzP4RmlbFbcZO4YRSsSZWSLCKfSEmUvaeLftE+RbYkX3nZxfqxH+6xpFNDiKBx MwpncYC0ZuzngKWJShbL2TKEnL8E6Mon0TIDqCj1ejC/HeX44cEQEPUQ12GqyByN X0maUSS81GoXzOGalL9ukveQgrs= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:in-reply-to:references:date:message-id :mime-version:content-type; s=default; bh=tDMa/vIcIa0ZoB+5EsRZWg n1mbE=; b=ES5aO2RA+tNhUbmynBFvvlDapPuysJtY+OTouFHuvok1mulKKSuzNe 3sMC+clnyaV8RaMbNHwqpQyRo7e/PTQOVRjsCxhkZw2SoyRlnNr7U6zAjkY8KNjP TcEvOkHJ5khaUw3K9r/67iioWqtfyi2leWwx0BYGYqsYEHikKpVYo= Received: (qmail 58120 invoked by alias); 18 Jun 2019 23:06:46 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 58106 invoked by uid 89); 18 Jun 2019 23:06:45 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-16.7 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.1 spammy=ns, brown, Brown 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; Tue, 18 Jun 2019 23:06:43 +0000 Received: from svr-orw-mbx-05.mgc.mentorg.com ([147.34.90.205]) by relay1.mentorg.com with esmtps (TLSv1.2:ECDHE-RSA-AES256-SHA384:256) id 1hdNBZ-00064F-Qd from Thomas_Schwinge@mentor.com ; Tue, 18 Jun 2019 16:06:41 -0700 Received: from svr-orw-mbx-01.mgc.mentorg.com (147.34.90.201) by SVR-ORW-MBX-05.mgc.mentorg.com (147.34.90.205) with Microsoft SMTP Server (TLS) id 15.0.1320.4; Tue, 18 Jun 2019 16:06:39 -0700 Received: from tftp-cs (147.34.91.1) by svr-orw-mbx-01.mgc.mentorg.com (147.34.90.201) with Microsoft SMTP Server id 15.0.1320.4 via Frontend Transport; Tue, 18 Jun 2019 16:06:39 -0700 Received: by tftp-cs (Postfix, from userid 49978) id C94C2C2212; Tue, 18 Jun 2019 16:06:38 -0700 (PDT) From: Thomas Schwinge To: , , Julian Brown CC: Jakub Jelinek , Bernhard Reutner-Fischer Subject: [committed] [PR90921] Fortran OpenACC 'declare' directive's module handling causes duplicate data clauses (was: [PATCH, OpenACC] Fortran "declare create"/allocate support for OpenACC) In-Reply-To: <20181004140413.2147eca1@squid.athome> References: <20180920195908.04486d45@squid.athome> <20180921031422.5a080b4a@nbbrfq.loc> <20180921183159.2275bb0c@squid.athome> <20181004140413.2147eca1@squid.athome> User-Agent: Notmuch/0.9-125-g4686d11 (http://notmuchmail.org) Emacs/26.1 (x86_64-pc-linux-gnu) Date: Wed, 19 Jun 2019 01:06:19 +0200 Message-ID: <87h88mfq8k.fsf@euler.schwinge.homeip.net> MIME-Version: 1.0 Hi! On Thu, 4 Oct 2018 14:04:13 +0100, Julian Brown wrote: > On Sun, 23 Sep 2018 10:48:52 +0200 > Bernhard Reutner-Fischer wrote: > > > On Sat, 22 Sep 2018 at 00:32, Julian Brown > > wrote: > > > > @@ -6218,13 +6221,20 @@ add_clause (gfc_symbol *sym, gfc_omp_map_op > > map_op) { > > gfc_omp_namelist *n; > > > > + if (!module_oacc_clauses) > > + module_oacc_clauses = gfc_get_omp_clauses (); > > + > > + if (sym->backend_decl == NULL) > > + gfc_get_symbol_decl (sym); > > + > > + for (n = module_oacc_clauses->lists[OMP_LIST_MAP]; n != NULL; n = > > n->next) > > + if (n->sym->backend_decl == sym->backend_decl) > > + return; > > + > > > > Didn't look too close, but should this throw an error instead of > > silently returning, or was the error emitted earlier? Bernhard, thanks for pointing out this "smelly" code, and then Julian for analyzing the actual issue: > The purpose of this fragment seems not to have been to do with error > reporting at all, but rather to do with de-duplicating symbols that > are listed (once) in clauses of "declare" directives in module blocks. > Variables that are listed twice are diagnosed elsewhere. > > As for why the de-duplication is necessary, it seems to be because of > the way that modules are instantiated in programs and in subroutines. > E.g. in declare-allocatable-1.f90, we have something along the lines of: > > module vars > implicit none > integer, parameter :: n = 100 > real*8, allocatable :: b(:) > !$acc declare create (b) > end module vars > > program test > use vars > ... > end program test > > subroutine sub1 > use vars > ... > end subroutine sub1 > > subroutine sub2 > use vars > ... > end subroutine sub2 > > The function find_module_oacc_declare_clauses is called for each of > 'test', 'sub1' and 'sub2'. But in trans-decl.c:finish_oacc_declare, the > new declare clauses are only attached to the namespace for a FL_PROGRAM > (i.e. 'test'), not for the subroutines. The module_oacc_clauses global > variable is reset only after moving the clauses to a FL_PROGRAM's > namespace, otherwise it accumulates. > > Hence, with the above code, we'd scan 'test', find declare clauses, and > attach them to the namespace for 'test'. We'd then reset > module_oacc_clauses. > > Then, we'd scan 'sub1', and accumulate declare clauses from 'vars' into > a fresh module_oacc_clauses. > > Then we'd scan 'sub2', and accumulate declare clauses from 'vars' > again: this is why the de-duplication in the patch seemed to be > necessary. > > This seems wrong to me though, and admits the possibility of clauses > instantiated in a subroutine "leaking" into a subsequent program block. > As a tentative fix, I've tried resetting module_oacc_clauses before > each time the find_module_oacc_declare_clauses traversal takes place, > and removing the de-duplication code. So, that's clearly a separate bug from everything else discussed as part of this patch submission; "Fortran OpenACC 'declare' directive's module handling causes duplicate data clauses" filed. > This seems to work fine for the current tests in the testsuite, but I > wonder the reason that things weren't done like like that to start > with? The code dates back to 2015 (by James Norris): > > https://gcc.gnu.org/ml/gcc-patches/2015-11/msg02367.html There remains a lot of mystery to be resolved regarding the OpenACC 'declare' implementation... :-( > --- a/gcc/fortran/trans-decl.c > +++ b/gcc/fortran/trans-decl.c > @@ -6272,6 +6275,8 @@ finish_oacc_declare (gfc_namespace *ns, gfc_symbol *sym, bool block) > gfc_omp_clauses *omp_clauses = NULL; > gfc_omp_namelist *n, *p; > > + module_oacc_clauses = NULL; > + > gfc_traverse_ns (ns, find_module_oacc_declare_clauses); > > if (module_oacc_clauses && sym->attr.flavor == FL_PROGRAM) > @@ -6283,7 +6288,6 @@ finish_oacc_declare (gfc_namespace *ns, gfc_symbol *sym, bool block) > new_oc->clauses = module_oacc_clauses; > > ns->oacc_declare = new_oc; > - module_oacc_clauses = NULL; > } > > if (!ns->oacc_declare) I cannot claim to understand this Fortran OpenACC 'declare' directive's module handling here, but I can at least confirm via a test case that I've added, that your change makes the duplicate data clauses go away; committed to trunk in r272454 "[PR90921] Fortran OpenACC 'declare' directive's module handling causes duplicate data clauses", see attached. Grüße Thomas From 9f15ed31065cf6baaae9b3e0e4c16fb9e958fbd9 Mon Sep 17 00:00:00 2001 From: tschwinge Date: Tue, 18 Jun 2019 22:15:53 +0000 Subject: [PATCH] [PR90921] Fortran OpenACC 'declare' directive's module handling causes duplicate data clauses gcc/fortran/ PR fortran/90921 * trans-decl.c (finish_oacc_declare): Reset module_oacc_clauses before scanning each namespace. gcc/testsuite/ PR fortran/90921 * gfortran.dg/goacc/declare-3.f95: Update. git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@272454 138bc75d-0d04-0410-961f-82ee72b054a4 --- gcc/fortran/ChangeLog | 6 ++++++ gcc/fortran/trans-decl.c | 2 +- gcc/testsuite/ChangeLog | 3 +++ gcc/testsuite/gfortran.dg/goacc/declare-3.f95 | 6 ++++++ 4 files changed, 16 insertions(+), 1 deletion(-) diff --git a/gcc/fortran/ChangeLog b/gcc/fortran/ChangeLog index 6fd97b61ce05..32d961ade960 100644 --- a/gcc/fortran/ChangeLog +++ b/gcc/fortran/ChangeLog @@ -1,3 +1,9 @@ +2019-06-18 Julian Brown + + PR fortran/90921 + * trans-decl.c (finish_oacc_declare): Reset module_oacc_clauses + before scanning each namespace. + 2019-06-18 Thomas Schwinge PR fortran/85221 diff --git a/gcc/fortran/trans-decl.c b/gcc/fortran/trans-decl.c index f504c279c31b..64ce4bba23d9 100644 --- a/gcc/fortran/trans-decl.c +++ b/gcc/fortran/trans-decl.c @@ -6491,6 +6491,7 @@ finish_oacc_declare (gfc_namespace *ns, gfc_symbol *sym, bool block) gfc_omp_clauses *omp_clauses = NULL; gfc_omp_namelist *n, *p; + module_oacc_clauses = NULL; gfc_traverse_ns (ns, find_module_oacc_declare_clauses); if (module_oacc_clauses && sym->attr.flavor == FL_PROGRAM) @@ -6502,7 +6503,6 @@ finish_oacc_declare (gfc_namespace *ns, gfc_symbol *sym, bool block) new_oc->clauses = module_oacc_clauses; ns->oacc_declare = new_oc; - module_oacc_clauses = NULL; } if (!ns->oacc_declare) diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog index 552ccc6fbd68..6ff197c8e4dc 100644 --- a/gcc/testsuite/ChangeLog +++ b/gcc/testsuite/ChangeLog @@ -1,5 +1,8 @@ 2019-06-18 Thomas Schwinge + PR fortran/90921 + * gfortran.dg/goacc/declare-3.f95: Update. + PR fortran/85221 * gfortran.dg/goacc/declare-3.f95: New file. diff --git a/gcc/testsuite/gfortran.dg/goacc/declare-3.f95 b/gcc/testsuite/gfortran.dg/goacc/declare-3.f95 index ec5d4c5a062a..80d9903a9dc6 100644 --- a/gcc/testsuite/gfortran.dg/goacc/declare-3.f95 +++ b/gcc/testsuite/gfortran.dg/goacc/declare-3.f95 @@ -1,5 +1,7 @@ ! Test valid usage of the OpenACC 'declare' directive. +! { dg-additional-options "-fdump-tree-original" } + module mod_a implicit none integer :: a @@ -44,4 +46,8 @@ program test use mod_c use mod_d use mod_e + + ! { dg-final { scan-tree-dump {(?n)#pragma acc data map\(force_alloc:d\) map\(force_deviceptr:c\) map\(force_to:b\) map\(force_alloc:a\)$} original } } end program test + +! { dg-final { scan-tree-dump-times {#pragma acc data} 1 original } } -- 2.20.1