From patchwork Thu Apr 20 21:33:40 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Cesar Philippidis X-Patchwork-Id: 753020 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org 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 3w8BvZ2THpz9s4s for ; Fri, 21 Apr 2017 07:34:02 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="IFvN6T/R"; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:from:message-id:date:mime-version :in-reply-to:content-type; q=dns; s=default; b=rwPp4RglLxgC8qJvh qqqFEl2KdcqnZBmBDXSVAQ1j4adsBuZ0hQ6CsdkRuq461/ITd7Lcr6ZqNMbzlemA RZ3I0TOgUBEGB6enT/VA53iEL+dmVYaPGXAQMhpVh3pJmki9huhrHwHJxKJC1iwl TTCR0uVhcuNxtDMBmJIR4YyVPM= 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 :subject:to:references:from:message-id:date:mime-version :in-reply-to:content-type; s=default; bh=H8oIiQyDCtRdOemeoXWtPUV uGig=; b=IFvN6T/RvYb6O4oSegZErrDnDEmuU4SPWLbkKVFTz9EfjJ3mHDCHBnd PW4RO/yUs8XzlNsHWbNW9/YTqlifoSlfa5+vmpXB+o0bzNbcJ1+wcagZrJuGUWhH nPoDi7ttuPCy1w6pwZRljKjF2CwtUPqxOpno8iMm55q5oCw3OyqI= Received: (qmail 64218 invoked by alias); 20 Apr 2017 21:33:47 -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 64177 invoked by uid 89); 20 Apr 2017 21:33:46 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.6 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS, URIBL_RED autolearn=ham version=3.3.2 spammy= 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; Thu, 20 Apr 2017 21:33:44 +0000 Received: from svr-orw-mbx-04.mgc.mentorg.com ([147.34.90.204]) by relay1.mentorg.com with esmtp id 1d1Jhv-0000Eo-96 from Cesar_Philippidis@mentor.com ; Thu, 20 Apr 2017 14:33:43 -0700 Received: from [127.0.0.1] (147.34.91.1) by SVR-ORW-MBX-04.mgc.mentorg.com (147.34.90.204) with Microsoft SMTP Server (TLS) id 15.0.1210.3; Thu, 20 Apr 2017 14:33:40 -0700 Subject: Re: [gomp4] add support for allocatable scalars in OpenACC declare constructs To: Thomas Schwinge , "gcc-patches@gcc.gnu.org" , Fortran List References: <486f1f87-16b3-46b5-17f5-6857756d4115@codesourcery.com> <871ssnfqn0.fsf@euler.schwinge.homeip.net> From: Cesar Philippidis Message-ID: Date: Thu, 20 Apr 2017 14:33:40 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <871ssnfqn0.fsf@euler.schwinge.homeip.net> X-ClientProxiedBy: SVR-ORW-MBX-07.mgc.mentorg.com (147.34.90.207) To SVR-ORW-MBX-04.mgc.mentorg.com (147.34.90.204) On 04/20/2017 01:08 AM, Thomas Schwinge wrote: > On Wed, 19 Apr 2017 11:11:39 -0700, Cesar Philippidis wrote: >> Included in this patch is a bug fix for non-declared allocatable >> scalars. [...] > > Please, bug fixes as work items/patches/commits separate from new > features. (As long as that makes sense.) I tried, but these were too closely related. >> + if (code->op == EXEC_OACC_UPDATE) >> + for (tree c = oacc_clauses; c; c = OMP_CLAUSE_CHAIN (c)) >> + { >> + if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP >> + && OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_FIRSTPRIVATE_POINTER) >> + OMP_CLAUSE_SET_MAP_KIND (c, GOMP_MAP_ALWAYS_POINTER); >> + } > >> --- a/gcc/testsuite/gfortran.dg/goacc/declare-allocatable-1.f90 >> +++ b/gcc/testsuite/gfortran.dg/goacc/declare-allocatable-1.f90 >> @@ -6,20 +6,20 @@ >> >> program allocate >> implicit none >> - integer, allocatable :: a(:) >> + integer, allocatable :: a(:), b >> integer, parameter :: n = 100 >> integer i >> - !$acc declare create(a) >> + !$acc declare create(a,b) >> >> - allocate (a(n)) >> + allocate (a(n), b) >> >> - !$acc parallel loop copyout(a) >> + !$acc parallel loop copyout(a, b) >> do i = 1, n >> - a(i) = i >> + a(i) = b >> end do > > That "a(i) = b" assignment is reading uninitialized data ("copyout(b)"). > Did you mean to specify "copyin(b)" or (implicit?) "firstprivate(b)", and > set "b" to a defined value before the region? This is only a compiler test and all of the interesting stuff happens during gimplication. In fact, this test exposed an ICE while testing allocatable scalars early on. >> - deallocate (a) >> + deallocate (a, b) >> end program allocate >> >> -! { dg-final { scan-tree-dump-times "pragma acc enter data map.declare_allocate" 1 "original" } } >> -! { dg-final { scan-tree-dump-times "pragma acc exit data map.declare_deallocate" 1 "original" } } >> +! { dg-final { scan-tree-dump-times "pragma acc enter data map.declare_allocate" 2 "original" } } >> +! { dg-final { scan-tree-dump-times "pragma acc exit data map.declare_deallocate" 2 "original" } } > >> --- /dev/null >> +++ b/libgomp/testsuite/libgomp.oacc-fortran/allocatable-scalar.f90 >> @@ -0,0 +1,30 @@ > > Missing "{ dg-do run }" to exercise torture testing -- or is that > intentionally not done here? Corrected in the attached gomp4-allocate-test.diff. >> +program main >> + implicit none >> + integer, parameter :: n = 100 >> + integer, allocatable :: a, c >> + integer :: i, b(n) >> + >> + allocate (a) >> + >> + a = 50 >> + >> + !$acc parallel loop >> + do i = 1, n; >> + b(i) = a >> + end do >> + >> + do i = 1, n >> + if (b(i) /= a) call abort >> + end do >> + >> + allocate (c) >> + >> + print *, loc (c) >> + !$acc parallel copyout(c) num_gangs(1) >> + c = a >> + !$acc end parallel >> + >> + if (c /= a) call abort >> + >> + deallocate (a, c) >> +end program main > > > Additionally, I'm seeing one regression: > > [-PASS:-]{+FAIL: gfortran.dg/goacc/pr77371-1.f90 -O (internal compiler error)+} > {+FAIL:+} gfortran.dg/goacc/pr77371-1.f90 -O (test for excess errors) > > [...]/gcc/testsuite/gfortran.dg/goacc/pr77371-1.f90:5:0: internal compiler error: in force_constant_size, at gimplify.c:664 > 0x87c57b force_constant_size > [...]/gcc/gimplify.c:664 > 0x87e687 gimple_add_tmp_var(tree_node*) > [...]/gcc/gimplify.c:702 > 0x867f3d create_tmp_var(tree_node*, char const*) > [...]/gcc/gimple-expr.c:476 > 0x9a1b95 lower_omp_target > [...]/gcc/omp-low.c:16875 > [...] That's because lower_omp_target is now allocating local storage for pointers, and that ICE is triggered by creating a temporary variable for a VLA. I don't think VLAs are supported on accelerators because of the lack of alloca, so I just fell back to the original behavior of not allocating local storage for that variable. See gomp4-pr77371-1-ice.diff for more details. Maybe the gimplifier should emit a warning when it encounters such a variable? Another thing we can do is teach GCC how to upload firstprivate variables into const memory, so that user cannot manipulate them. But that doesn't help improve the correctness of the program. That lower_omp_target patch is still under test. I'll apply gomp4-allocate-test.diff to gomp-4_0-branch shortly. Cesar 2017-04-20 Cesar Philippidis libgomp/ * testsuite/libgomp.oacc-fortran/allocatable-scalar.f90: Clean up test. diff --git a/libgomp/testsuite/libgomp.oacc-fortran/allocatable-scalar.f90 b/libgomp/testsuite/libgomp.oacc-fortran/allocatable-scalar.f90 index 8386c5d..4af42bc 100644 --- a/libgomp/testsuite/libgomp.oacc-fortran/allocatable-scalar.f90 +++ b/libgomp/testsuite/libgomp.oacc-fortran/allocatable-scalar.f90 @@ -1,3 +1,7 @@ +! Test non-declared allocatable scalars in OpenACC data clauses. + +! { dg-do run } + program main implicit none integer, parameter :: n = 100 @@ -19,7 +23,6 @@ program main allocate (c) - print *, loc (c) !$acc parallel copyout(c) num_gangs(1) c = a !$acc end parallel