From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.120]) by sourceware.org (Postfix) with ESMTP id 7DB613951C26 for ; Mon, 31 Aug 2020 16:34:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 7DB613951C26 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-16-uVWNjp11MEWh8WmkGhFlhg-1; Mon, 31 Aug 2020 12:34:40 -0400 X-MC-Unique: uVWNjp11MEWh8WmkGhFlhg-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 52121106658B; Mon, 31 Aug 2020 16:34:39 +0000 (UTC) Received: from tucnak.zalov.cz (ovpn-113-115.ams2.redhat.com [10.36.113.115]) by smtp.corp.redhat.com (Postfix) with ESMTPS id D1D4D60C15; Mon, 31 Aug 2020 16:34:38 +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 07VGYZU8015687; Mon, 31 Aug 2020 18:34:35 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.15.2/8.15.2/Submit) id 07VGYYIX015686; Mon, 31 Aug 2020 18:34:34 +0200 Date: Mon, 31 Aug 2020 18:34:34 +0200 From: Jakub Jelinek To: Tobias Burnus Cc: gcc-patches , fortran Subject: Re: [Patch] OpenMP/Fortran: Handle polymorphic scalars in data-sharing FIRSTPRIVATE (PR86470) Message-ID: <20200831163434.GT18149@tucnak> Reply-To: Jakub Jelinek References: <8bcd00c3-9de5-c072-d893-9218a08a1c2f@codesourcery.com> MIME-Version: 1.0 In-Reply-To: <8bcd00c3-9de5-c072-d893-9218a08a1c2f@codesourcery.com> User-Agent: Mutt/1.11.3 (2019-02-01) X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Mimecast-Spam-Score: 0.002 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-3.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: fortran@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Fortran mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 31 Aug 2020 16:34:43 -0000 On Tue, Aug 25, 2020 at 12:50:46PM +0200, Tobias Burnus wrote: > OK for mainline? Generally, you know Fortran FE much more than I do, so just a few random comments. > --- a/gcc/fortran/trans-openmp.c > +++ b/gcc/fortran/trans-openmp.c > @@ -355,6 +355,51 @@ gfc_has_alloc_comps (tree type, tree decl) > return false; > } > > +/* Return true if TYPE is polymorphic but not with pointer attribute. */ > + > +static bool > +gfc_is_polymorphic_nonptr (tree type) > +{ > + if (POINTER_TYPE_P (type)) > + type = TREE_TYPE (type); > + if (TREE_CODE (type) != RECORD_TYPE) > + return false; > + > + tree field = TYPE_FIELDS (type); > + if (!field || 0 != strcmp ("_data", IDENTIFIER_POINTER (DECL_NAME (field)))) > + return false; > + field = DECL_CHAIN (field); > + if (!field || 0 != strcmp ("_vptr", IDENTIFIER_POINTER (DECL_NAME (field)))) Is it safe to just look at the field names? Shouldn't it at least also test that the fields are DECL_ARTIFICIAL, or somehow else ensure that it isn't a user derived type with _data and _vptr fields in it. type foo integer :: _data integer :: _vptr integer :: _len end type type(foo) :: a a%_data = 1 a%_vptr = 2 a%_len = 3 end compiles just fine with -fallow-leading-underscore ... > @@ -740,6 +785,87 @@ gfc_omp_clause_copy_ctor (tree clause, tree dest, tree src) > gcc_assert (OMP_CLAUSE_CODE (clause) == OMP_CLAUSE_FIRSTPRIVATE > || OMP_CLAUSE_CODE (clause) == OMP_CLAUSE_LINEAR); > > + /* TODO: implement support for polymorphic arrays; reject for now. */ > + /* Void arrays appear as var.0 = var._data.data. A bit hackish to > + distinguish from 'type(c_ptr) :: var(5)' by scanning for '.'; > + this assumes that ASM_FORMAT_PRIVATE_NAME uses a '.', which most > + systems do. */ > + if (TREE_CODE (type) == ARRAY_TYPE > + && TREE_TYPE (type) == pvoid_type_node > + && TREE_CODE (dest) == MEM_REF > + && strchr (IDENTIFIER_POINTER (DECL_NAME (TREE_OPERAND (dest, 0))), '.')) This seems very fragile, there are targets that use $ instead, other targets use only underscores. $ grep NO_DOT_IN_LABEL config/* config/*/* 2>/dev/null config/vx-common.h:# define NO_DOT_IN_LABEL config/mmix/mmix.h:#define NO_DOT_IN_LABEL config/nvptx/nvptx.h:#define NO_DOT_IN_LABEL config/xtensa/elf.h:#define NO_DOT_IN_LABEL $ grep -w NO_DOLLAR_IN_LABEL config/* config/*/* 2>/dev/null config/dragonfly.h:#undef NO_DOLLAR_IN_LABEL config/elfos.h:#define NO_DOLLAR_IN_LABEL config/freebsd.h:#undef NO_DOLLAR_IN_LABEL config/vx-common.h:# undef NO_DOLLAR_IN_LABEL config/alpha/alpha.h:#undef NO_DOLLAR_IN_LABEL config/arm/aout.h:#ifndef NO_DOLLAR_IN_LABEL config/arm/aout.h:#define NO_DOLLAR_IN_LABEL 1 config/mips/n32-elf.h:#define NO_DOLLAR_IN_LABEL config/mmix/mmix.h:#define NO_DOLLAR_IN_LABEL config/rs6000/rs6000.c:#ifdef NO_DOLLAR_IN_LABEL config/rs6000/rs6000-protos.h:#ifdef NO_DOLLAR_IN_LABEL config/rs6000/xcoff.h:#define NO_DOLLAR_IN_LABEL config/tilegx/tilegx.h:#undef NO_DOLLAR_IN_LABEL config/tilepro/tilepro.h:#undef NO_DOLLAR_IN_LABEL config/xtensa/elf.h:#undef NO_DOLLAR_IN_LABEL Couldn't it be recorded somewhere in DECL_LANG_SPECIFIC of the decl? > + fatal_error (input_location, > + "Sorry, polymorphic arrays not yet supported for " > + "firstprivate"); Shouldn't this be sorry ("...") instead? > + /* var._data - _data is void* for scalars and descriptor for arrays. */ > + if (TREE_CODE (TREE_TYPE (TYPE_FIELDS (type))) == RECORD_TYPE) > + fatal_error (input_location, > + "Sorry, polymorphic arrays not yet supported for " > + "firstprivate"); Likewise. > + /* Malloc memory + call class->_vpt->_copy. */ > + call = builtin_decl_explicit (BUILT_IN_MALLOC); Is malloc what the FE uses elsewhere for it? Will something free it afterwards? > + if (!GFC_DECL_GET_SCALAR_ALLOCATABLE (TREE_OPERAND (dest_data, 1))) > + { > + gfc_add_block_to_block (&block, &cond_block); > + } Formatting, one stmt shouldn't be wrapped into {}s. Jakub