From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from esa2.mentor.iphmx.com (esa2.mentor.iphmx.com [68.232.141.98]) by sourceware.org (Postfix) with ESMTPS id D93C03857820 for ; Fri, 20 May 2022 06:59:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D93C03857820 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,238,1647331200"; d="scan'208";a="76117874" Received: from orw-gwy-02-in.mentorg.com ([192.94.38.167]) by esa2.mentor.iphmx.com with ESMTP; 19 May 2022 22:59:19 -0800 IronPort-SDR: w0Hvoz+bPTBgxBI8xdhkL8aMXzKe6ylniiHRzZPv8nKRjLEemMEb7fwJ4/61+P2+0+kF0R9K0G CqJfjAv1ZH8WbXAxAHvg3kfqGjt+PtWFBAeqPX6e5pmL/lKFfOyKLK3UKDc+/vZ1gr8GYGn8sE KjO+SgrhCUSFsuEPZeb5P5qpKEeH6Co/p1Gj7bgemTwqeJyBjpLGlgJyBfVnIIfx3Cp+/GUWOT UK7YuzGQY2JS0PFWEpQBr3XifqPSyi0JeHfDhsg/zeCylBnKrr6eOzUgvp3jlm1oIJIDBySkAI 9/g= Message-ID: Date: Fri, 20 May 2022 08:59:14 +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, v2] Implement uses_allocators clause for target regions Content-Language: en-US To: Jakub Jelinek , Chung-Lin Tang CC: Tobias Burnus , gcc-patches , Catherine Moore , Andrew Stubbs , Hafiz Abid Qadeer References: <46d77e14-080c-db6c-4032-e12899c5d059@codesourcery.com> <9c0945fa-1054-095e-86ae-a9d8dd1ab625@codesourcery.com> From: Tobias Burnus In-Reply-To: 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-11.mgc.mentorg.com (139.181.222.11) To svr-ies-mbx-12.mgc.mentorg.com (139.181.222.12) X-Spam-Status: No, score=-5.8 required=5.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, NICE_REPLY_A, SPF_HELO_PASS, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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, 20 May 2022 06:59:23 -0000 Hi Jakub, On 19.05.22 18:00, Jakub Jelinek wrote: > On Tue, May 10, 2022 at 07:29:23PM +0800, Chung-Lin Tang wrote: >> I've attached v2 of the patch. Currently in testing. > Just a general rant, the non-requires dynamic_allocators support > seems to be a total mess in the standard. Probably something > that should be discussed on omp-lang. Or in some issue. Some newer developments (all links unfortunately nonpublic): There is now a nearly ready example for the 5.2.1 example document, cf. https://github.com/OpenMP/examples-internal/issues/275 https://github.com/OpenMP/spec/issues/3229 improves the wording related to 'requires dynamic_allocators' =E2=80=93 vote was after the 5.2 release. > [...]Allocators can appear in various places, with requires dynamic_alloc= ators > it is all clear, the only allocators required to be constant expressions > (aka predefined allocators) are on allocate for non-automatic variables Side remark: the OMP_ALLOCATOR environment variable sets the def-allocator-var ICV =E2=80=93 but besides pre-defined allocators, it also permits to define (traits, memspace, ...) a new default allocator (new in O= MP 5.1). And this one can then seemingly also be used in the target region (only with 'requires dynamic_allocators'). I note that GCC supports OMP_ALLOCATOR but seemingly only with predefined allocators (=E2=86=92 OMP 5.0). =E2=80=93 I think we need to ope= n PR for that one and/or a new line in the 5.1 implementation tables. > (my understanding is that omp_null_allocator isn't valid for those but I > could be wrong), (I read it likewise as it is not predefined,) > Without requires dynamic_allocators, there are various extra restrictions > imposed: > 1) omp_init_allocator/omp_destroy_allocator [...] > 2) omp_alloc etc. [...] > 3) for allocate directive on static vars [...] > 4) for allocate clause e.g. when privatizing stuff or allocate directive > for automatic vars no such restrictions exist > > Now, that means that e.g. the user provided uses_allocators without > requires dynamic_allocators are only useful for the case 4), it is unclea= r > if that was really intended. Note that (4) not only applies for 'allocate' on the 'target' construct but it can be also be used on any other directive inside the target construct, i.e.: #pragma omp target uses_allocators(omp_cgroup_mem_alloc) #pragma omp teams reduction(+:xbuf) thread_limit(N) \ allocate(omp_cgroup_mem_alloc:xbuf) (from the example issue, linked above). > With uses_allocators in particular, it is unclear if > uses_allocators(omp_null_allocator) is allowed or not, IMHO it shouldn't, > but I really don't see a restriction disallowing it. I think it does not count as predefined allocator and the (new, post-5.2) wording makes clear that the default allocator (which is associated with the omp_null_allocator per wording in, e.g., omp_alloc) is only valid with 'dynamic_allocators'. > Then there is the issue that 5.0/5.1 said for C/C++ that traits-array > should be > "an identifier of const omp_alloctrait_t * type." > which is wrong for multiple reasons, because identifiers don't have type, > expressions or variables do, but more importantly because from the pointe= r > to const omp_alloctrait_t it is impossible to find out how many elements > the traits have. 5.2 fixed that to say that it must be an array > (so we thankfully know the size), so we certainly should consider that > change like a defect report against 5.0/5.1 too and require even in the > old syntax an array. Note, I'm afraid we need to support even VLAs, > not just constant size arrays. I concur that 5.2 should be regarded both as fix of old bugs and syntax extension. (Additionally, as we already support the 5.2 syntax.) There were some improvements discussed/proposed in https://github.com/OpenMP/spec/issues/3285 It is a bit difficult to read as I confused two things at the beginning (mixing allocator expression / traits argument when reading the bullet points). =E2=80=93 But it relates to some of the items you raised here. > There is also in the spec that when allocator in uses_allocators is > a variable, it is treated as a private variable that can't be explicitly > privatized, but nothing said about the traits array, so is say: > void foo () { > omp_allocator_handle_t h; > omp_alloctrait_t t[3] =3D { ... }; > #pragma omp target uses_allocators(h(t)) firstprivate(t) > { > } > ok or not? (try in addition 'allocate(h : t)' ) > We need to firstprivatize t so that we can call > h =3D omp_init_allocator (omp_default_mem_space, 3, t); in the target reg= ion > and it is kind of difficult to privatize the same var multiple times. I think this relates to the generic question related to mapping/firstprivatizing const/constexpr/PARAMETER etc. https://github.com/OpenMP/spec/issues/2158 which really should be fixed. > And yet another issue, in omp_alloctrait_t one can point to other allocat= ors > (with { omp_atk_fallback, some_alloc }). If some_alloc is a predefined > allocator, fine, I don't see big deal with that, especially if that > predefined allocator is also mentioned in uses_allocators clause (before = or > after). But if it is a user allocator, there is no restriction on that, = and > no way how to map that, say that there should be some specific ordering > of uses_allocators induced omp_init_allocator calls and that we should > somehow replace the host value with privatized target replacement. I think that might need a clarification/fix on the OpenMP spec side. 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