From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from esa3.mentor.iphmx.com (esa3.mentor.iphmx.com [68.232.137.180]) by sourceware.org (Postfix) with ESMTPS id EFF123858C27 for ; Fri, 6 May 2022 16:40:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org EFF123858C27 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=codesourcery.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=mentor.com X-IronPort-AV: E=Sophos;i="5.91,203,1647331200"; d="scan'208";a="75242363" Received: from orw-gwy-02-in.mentorg.com ([192.94.38.167]) by esa3.mentor.iphmx.com with ESMTP; 06 May 2022 08:40:51 -0800 IronPort-SDR: iIWCz5ZiFy1z5O9JNmUDvxtDJQsIz2svCBhxKWBZXK0GlP5Xtokp1ejWnDef9k59YJFsmqZX68 cnW+LVgTBvB6NpxpiN9i7//CTAtdsjQJiVWpDjUfHpbXQ7ZWQLR+lekUcUaaVvyCtfYXo8gyV1 gDXalAimPV9Q405OgrQn2+0bk1amxlK3BZm41YhJ2OCMTyGbVzEwBaIhmcN1MXqclKeOENLP65 zKrRTfA1aqU6jAwFFiwVcNF5qdk0wzE0OXPuVbzV1QwGDlNonUkOt+oIZSPMmLpw33KZAgfWqP biM= Message-ID: <9c0945fa-1054-095e-86ae-a9d8dd1ab625@codesourcery.com> Date: Fri, 6 May 2022 18:40:43 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.0 Subject: Re: [PATCH, OpenMP] Implement uses_allocators clause for target regions Content-Language: en-US To: Chung-Lin Tang , gcc-patches , Jakub Jelinek , Tobias Burnus , Catherine Moore , Andrew Stubbs , Hafiz Abid Qadeer References: <46d77e14-080c-db6c-4032-e12899c5d059@codesourcery.com> From: Tobias Burnus In-Reply-To: <46d77e14-080c-db6c-4032-e12899c5d059@codesourcery.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: quoted-printable X-Originating-IP: [137.202.0.90] X-ClientProxiedBy: svr-ies-mbx-10.mgc.mentorg.com (139.181.222.10) To svr-ies-mbx-12.mgc.mentorg.com (139.181.222.12) X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, KAM_MANYTO, NICE_REPLY_A, SPF_HELO_PASS, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 06 May 2022 16:40:53 -0000 Hi Chung-Lin, thanks for the patch =E2=80=93 and some comments from my side. On 06.05.22 15:20, Chung-Lin Tang wrote: > For user defined allocator handles, this allows target regions to assign > memory space and traits to allocators, and automatically calls > omp_init/destroy_allocator() in the beginning/end of the target region. Can please also handle the new clause in Fortran's dump-parse-tree.cc? I did see some split handling in C, but not in Fortran; do you also need to up update gfc_split_omp_clauses in Fortran's trans-openmp.cc? Actually, glancing at the testcases, no combined construct (like "omp target parallel") is used, I think that would be useful because of =E2= =86=91. > +/* OpenMP 5.2: > + uses_allocators ( allocator-list ) That's not completely true: uses_allocators is OpenMP 5.1. However, 5.1 only supports (for non-predefined allocators): uses_allocators( allocator(traits) ) while OpenMP 5.2 added modifiers: uses_allocatrors( traits(...), memspace(...) : allocator ) and deprecated the 5.1 'allocator(traits)'. (Scheduled for removal in OMP 6= .0) The advantage of 5.2 syntax is that a memory space can be defined. BTW: This makes uses_allocators the first OpenMP 5.2 feature which will make it into GCC :-) gcc/fortran/openmp.cc: > + if (gfc_get_symbol ("omp_allocator_handle_kind", NULL, &sym) > + || !sym->value > + || sym->value->expr_type !=3D EXPR_CONSTANT > + || sym->value->ts.type !=3D BT_INTEGER) > + { > + gfc_error ("OpenMP % constant not foun= d by " > + "% clause at %C"); > + goto error; > + } > + allocator_handle_kind =3D sym; I think you rather want to use gfc_find_symbol ("omp_...", NULL, true, &sym) || sym =3D=3D NULL where true is for parent_flag to search also the parent namespace. (The function returns 1 if the symbol is ambiguous, 0 otherwise - including 0 + sym =3D=3D NULL when the symbol could not be found.) || sym->attr.flavor !=3D FL_PARAMETER || sym->ts.type !=3D BT_INTEGER || sym->attr.dimension Looks cleaner than to access sym->value. The attr.dimension is just to makes sure the user did not smuggle an array into this. (Invalid as omp_... is a reserved namespace but users will still do this and some are good in finding ICE as hobby.) * * * However, I fear that will fail for the following two examples (both unteste= d): use omp_lib, my_kind =3D omp_allocator_handle_kind integer(my_kind) :: my_allocator as this gives 'my_kind' in the symtree->name (while symtree->n.sym->name is= "omp_..."). Hence, by searching the symtree for 'omp_...' the symbol will not be found. It will likely also fail for the following more realistic example: module m use omp_lib implicit none private integer(omp_allocator_handle_kind), public :: my_allocator type(omp_alloctrait), public, parameter :: my_traits(*) =3D [...] end module subroutine foo use m use omp_lib, only: omp_alloctrait implicit none ! currently, same scope is required - makes sense for C and 'const' but ! not for Fortran's parameters; restriction might be lifted/clarified in ! next OpenMP version: type(omp_alloctrait), parameter :: traits_array(*) =3D my_traits integer :: A(200) A =3D 0 !$omp target uses_allocators(my_allocator(traits_array) allocate(my_allo= cator:A) firstprivate(A) ... !$omp end target end In this case, omp_allocator_handle_kind is not in the namespace of 'foo' but the code should be still valid. Thus, an alternative would be to hard-c= ode the value - as done for the depobj. As we have: integer, parameter :: omp_allocator_handle_kind =3D c_intptr_t integer, parameter :: omp_memspace_handle_kind =3D c_intptr_t that would be sym->ts.type =3D=3D BT_CHARACTER sym->ts.kind =3D=3D gfc_index_integer_kind for the allocator variable and the the memspace kind. However, I grant that either example is not very typical. The second one is= more natural =E2=80=93 such a code will very likely be written in the real world= . But not with uses_allocators but rather with "!$omp requires dynamic_allocators" an= d omp_init_allocator(). Thoughts? * * * gcc/fortran/openmp.cc > + if (++i > 2) > + { > + gfc_error ("Only two modifiers are allowed on %= " > + "clause at %C"); > + goto error; > + } > + Is this really needed? There is a check for multiple traits and multiple me= mspace Thus, 'trait(),memspace(),trait()' is already handled and 'trait(),something' give a break and will lead to an error as in that case a ':' and not ',something' is expected. > + if (gfc_match_char ('(') =3D=3D MATCH_YES) > + { > + if (memspace_seen || traits_seen) > + { > + gfc_error ("Modifiers cannot be used with legacy " > + "array syntax at %C"); I wouldn't uses the term 'array synax' to denote uses_allocators(allocator (alloc_array) ) How about: error: "Using both modifiers and allocator variable with traits argument= " (And I think 'deprecated' is better than 'legacy', if we really want to use= it.) > + if (traits_sym->ts.type !=3D BT_DERIVED > + || strcmp (traits_sym->ts.u.derived->name, > + "omp_alloctrait") !=3D 0 > + || traits_sym->attr.flavor !=3D FL_PARAMETER > + || traits_sym->as->rank !=3D 1 > + || traits_sym->value =3D=3D NULL > + || !gfc_is_constant_expr (traits_sym->value)) I think the gfc_is_constant_expr is unreachable as you already have checked FL_PARAMETER. Thus, you can remove the last two lines. [Regarding the traits_sym->ts.u.derived->name, I am not sure whether that won't fail with use omp_lib, trait_t =3D> omp_alloctrait but I have not checked. It likely does work correctly.] > + /* Check if identifier is of 'omp_..._mem_space' format. */ > + || (pos =3D strstr (memspace_sym->name, "omp_")) =3D=3D NULL > + || pos !=3D memspace_sym->name > + || (pos =3D strstr (memspace_sym->name, "_mem_space")) =3D=3D= NULL > + || *(pos + strlen ("_mem_space")) !=3D '\0') I wonder whether that's not more readable written as: || !startswith (memspace_sym->name, "omp_") || !endswith (memspace_sym->name, "_mem_space") Tobias ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstra=C3=9Fe 201= , 80634 M=C3=BCnchen; Gesellschaft mit beschr=C3=A4nkter Haftung; Gesch=C3= =A4ftsf=C3=BChrer: Thomas Heurung, Frank Th=C3=BCrauf; Sitz der Gesellschaf= t: M=C3=BCnchen; Registergericht M=C3=BCnchen, HRB 106955