From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 103578 invoked by alias); 5 Dec 2019 15:16:05 -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 103374 invoked by uid 89); 5 Dec 2019 15:15:51 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-2.8 required=5.0 tests=AWL,BAYES_00,KAM_SHORT autolearn=unavailable version=3.3.1 spammy= X-HELO: us-smtp-delivery-1.mimecast.com Received: from us-smtp-2.mimecast.com (HELO us-smtp-delivery-1.mimecast.com) (207.211.31.81) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 05 Dec 2019 15:15:27 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1575558925; h=from:from:reply-to:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=5Bdhnwas4iUETzTpvx2ej9szd1zRrER9tAk6SJfYuk0=; b=Cq6X/omYKfO/N+DCapPjwEtXdGKeTTh5vNicfL4HAqoAAF1mR7ykrfbCjNmCzYOs4UJxx+ 87b3kj3+7PV1UaDO4QY1FAVKpS1hlsTjTovtapzqAb5BJnbKjHR9ZKx63Sv9yU6UUOz3Cu gk+c2h2/Gpboo3424E+303ZdjSy6zWk= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-323-ruoWHZvBN8Cz7W6wpgjYRQ-1; Thu, 05 Dec 2019 10:15:21 -0500 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 6E84A101F504; Thu, 5 Dec 2019 15:15:20 +0000 (UTC) Received: from tucnak.zalov.cz (ovpn-117-59.ams2.redhat.com [10.36.117.59]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 00B1219C7F; Thu, 5 Dec 2019 15:15:19 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.15.2/8.15.2) with ESMTP id xB5FFHZO015283; Thu, 5 Dec 2019 16:15:17 +0100 Received: (from jakub@localhost) by tucnak.zalov.cz (8.15.2/8.15.2/Submit) id xB5FFFsg015282; Thu, 5 Dec 2019 16:15:15 +0100 Date: Thu, 05 Dec 2019 15:16:00 -0000 From: Jakub Jelinek To: Tobias Burnus Cc: gcc-patches , fortran , Thomas Schwinge Subject: Re: [Patch][OpenMP/OpenACC/Fortran] Fix mapping of optional (present|absent) arguments Message-ID: <20191205151515.GS10088@tucnak> Reply-To: Jakub Jelinek References: <8be82276-81b1-817c-fcd2-51f24f5fe2d2@codesourcery.com> MIME-Version: 1.0 In-Reply-To: <8be82276-81b1-817c-fcd2-51f24f5fe2d2@codesourcery.com> User-Agent: Mutt/1.11.3 (2019-02-01) X-Mimecast-Spam-Score: 0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: quoted-printable Content-Disposition: inline X-IsSubscribed: yes X-SW-Source: 2019-12/txt/msg00328.txt.bz2 On Wed, Nov 20, 2019 at 02:06:18PM +0100, Tobias Burnus wrote: > ** Included in the attached patch are the following previously posted > patches: [1] the trivial libgomp/oacc-mem.c change, [2] only the remaining > single-line change in omp-low.c, [3] the trans-openmp.c changes (which had > to be modified+extended), and [5] the test cases. ([2] and [4] are already > in GCC 10.) See: > https://gcc.gnu.org/ml/gcc-patches/2019-07/threads.html#00960 for the > original patches. >=20 > PS: For full OpenMP support, (absent) optional arguments also needed to be > handled for data-share clauses. Sure. > 2019-10-20 Tobias Burnus > Kwok Cheung Yeung >=20 > gcc/fortran/ > * trans-openmp.c (gfc_build_conditional_assign,=20 > gfc_build_conditional_assign_expr): New static functions. > (gfc_omp_finish_clause, gfc_trans_omp_clauses): Handle mapping of > absent optional arguments and fix mapping of present optional args. >=20 > gcc/ > * omp-low.c (lower_omp_target): For optional arguments, deref once > more to obtain the type. >=20 > libgomp/ > * oacc-mem.c (update_dev_host, gomp_acc_insert_pointer): Just return > if input it a NULL pointer. > * testsuite/libgomp.oacc-c-c++-common/lib-43.c: Remove; dependent on > diagnostic of NULL pointer. > * testsuite/libgomp.oacc-c-c++-common/lib-47.c: Ditto. > * testsuite/libgomp.fortran/optional-map.f90: New. > * testsuite/libgomp.fortran/use_device_addr-1.f90 > (test_dummy_opt_callee_1_absent): New. > (test_dummy_opt_call_1): Call it. > * testsuite/libgomp.fortran/use_device_addr-2.f90: Likewise. > * testsuite/libgomp.fortran/use_device_addr-3.f90: Likewise. > * testsuite/libgomp.fortran/use_device_addr-4.f90: Likewise. > * testsuite/libgomp.oacc-fortran/optional-cache.f95: New. > * testsuite/libgomp.oacc-fortran/optional-data-copyin-by-value.f90: New. > * testsuite/libgomp.oacc-fortran/optional-data-copyin.f90: New. > * testsuite/libgomp.oacc-fortran/optional-data-copyout.f90: New. > * testsuite/libgomp.oacc-fortran/optional-data-enter-exit.f90: New. > * testsuite/libgomp.oacc-fortran/optional-declare.f90: New. > * testsuite/libgomp.oacc-fortran/optional-firstprivate.f90: New. > * testsuite/libgomp.oacc-fortran/optional-host_data.f90: New. > * testsuite/libgomp.oacc-fortran/optional-nested-calls.f90: New. > * testsuite/libgomp.oacc-fortran/optional-private.f90: New. > * testsuite/libgomp.oacc-fortran/optional-reduction.f90: New. > * testsuite/libgomp.oacc-fortran/optional-update-device.f90: New. > * testsuite/libgomp.oacc-fortran/optional-update-host.f90: New. Ok, with some formatting nits fixed. > @@ -1199,6 +1257,8 @@ gfc_omp_finish_clause (tree c, gimple_seq *pre_p) > } >=20=20 > tree c2 =3D NULL_TREE, c3 =3D NULL_TREE, c4 =3D NULL_TREE; > + tree present =3D gfc_omp_is_optional_argument (decl) > + ? gfc_omp_check_optional_argument (decl, true) : NULL_TREE; I think emacs users (I'm not one of them) want ()s around, otherwise emacs misformats that. So tree present =3D (gfc_omp_is_optional_argument (decl) ? gfc_omp_check_optional_argument (decl, true) : NULL_TREE); > @@ -1232,17 +1314,43 @@ gfc_omp_finish_clause (tree c, gimple_seq *pre_p) > stmtblock_t block; > gfc_start_block (&block); > tree type =3D TREE_TYPE (decl); > - tree ptr =3D gfc_conv_descriptor_data_get (decl); > + tree ptr; > + > + if (present) > + ptr =3D gfc_build_conditional_assign_expr ( > + &block, present, > + gfc_conv_descriptor_data_get (decl), > + null_pointer_node); I must say I don't like very much formatting like that, I'd find it cleaner to use temporary to have shorter argument and put all the arguments after the ( column. > + else > + ptr =3D gfc_conv_descriptor_data_get (decl); In this case, it could even be: ptr =3D gfc_conv_descriptor_data_get (decl); if (present) ptr =3D gfc_build_conditional_assign_expr (&block, present, ptr, null_pointer_node); by just using the same call from the else. > @@ -2252,6 +2385,9 @@ gfc_trans_omp_clauses (stmtblock_t *block, gfc_omp_= clauses *clauses, > TREE_ADDRESSABLE (decl) =3D 1; > if (n->expr =3D=3D NULL || n->expr->ref->u.ar.type =3D=3D AR_FULL) > { > + tree present =3D gfc_omp_is_optional_argument (decl) > + ? gfc_omp_check_optional_argument (decl, true) > + : NULL_TREE; > if (POINTER_TYPE_P (TREE_TYPE (decl)) > && (gfc_omp_privatize_by_reference (decl) > || GFC_DECL_GET_SCALAR_POINTER (decl) See above. > @@ -2284,6 +2420,10 @@ gfc_trans_omp_clauses (stmtblock_t *block, gfc_omp= _clauses *clauses, > { > tree type =3D TREE_TYPE (decl); > tree ptr =3D gfc_conv_descriptor_data_get (decl); > + if (present) > + ptr =3D gfc_build_conditional_assign_expr ( > + block, present, ptr, > + null_pointer_node); And here the other comment. It is much more indented though, but you could use a temporary, like: tree nullarg =3D null_pointer_node; if (present) ptr =3D gfc_build_conditional_assign_expr (block, present, ptr, nullarg); Though, if it is too much for you, ignore. Another option would be shorten the name of the function, say s/conditional/cond/. There were some discussions about lifting the 80 column restriction and bump it to something like +-130, but nothing happened yet. > + ptr =3D gfc_build_conditional_assign_expr ( > + block, present, ptr, > + null_pointer_node); Again. > + stmtblock_t cond_block; > + gfc_init_block (&cond_block); > + tree size =3D gfc_full_array_size (&cond_block, decl, > + GFC_TYPE_ARRAY_RANK (type)); Here one could use a temporary for GFC_TYPE_ARRAY_RANK (type); > + if (present) > + { > + tree var =3D gfc_create_var (gfc_array_index_type, > + NULL); > + tree cond =3D fold_build2_loc (input_location, > + NE_EXPR, > + boolean_type_node, > + present, > + null_pointer_node); > + gfc_add_modify (&cond_block, var, size); > + gfc_add_expr_to_block (block, > + build3_loc (input_location, COND_EXPR, > + void_type_node, cond, > + gfc_finish_block (&cond_block), > + NULL_TREE)); And here for the expr, perhaps just reuse the cond variable. > @@ -2346,6 +2534,18 @@ gfc_trans_omp_clauses (stmtblock_t *block, gfc_omp= _clauses *clauses, > OMP_CLAUSE_SIZE (node), elemsz); > } > } > + else if (present > + && TREE_CODE (decl) =3D=3D INDIRECT_REF > + && TREE_CODE (TREE_OPERAND (decl, 0)) > + =3D=3D INDIRECT_REF) Above I'd expect && (TREE_CODE (TREE_OPERAND (decl, 0)) =3D=3D INDIRECT_REF)) but perhaps I'm just pushing my coding style too much, ignore in that case. Jakub