From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 39C0B395C007 for ; Thu, 19 May 2022 16:00:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 39C0B395C007 Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-367-mbEMUBlhO-S-ezWVEchfkA-1; Thu, 19 May 2022 12:00:10 -0400 X-MC-Unique: mbEMUBlhO-S-ezWVEchfkA-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 351EC801210; Thu, 19 May 2022 16:00:09 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.192.23]) by smtp.corp.redhat.com (Postfix) with ESMTPS id E10CD7AD5; Thu, 19 May 2022 16:00:08 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.16.1/8.16.1) with ESMTPS id 24JG050O076408 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Thu, 19 May 2022 18:00:06 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.16.1/8.16.1/Submit) id 24JG04Fn076407; Thu, 19 May 2022 18:00:04 +0200 Date: Thu, 19 May 2022 18:00:04 +0200 From: Jakub Jelinek To: Chung-Lin Tang Cc: Tobias Burnus , gcc-patches , Catherine Moore , Andrew Stubbs , Hafiz Abid Qadeer Subject: Re: [PATCH, OpenMP, v2] Implement uses_allocators clause for target regions Message-ID: Reply-To: Jakub Jelinek References: <46d77e14-080c-db6c-4032-e12899c5d059@codesourcery.com> <9c0945fa-1054-095e-86ae-a9d8dd1ab625@codesourcery.com> MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-4.1 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_NONE, 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: Thu, 19 May 2022 16:00:19 -0000 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. Allocators can appear in various places, with requires dynamic_allocators it is all clear, the only allocators required to be constant expressions (aka predefined allocators) are on allocate for non-automatic variables (my understanding is that omp_null_allocator isn't valid for those but I could be wrong), but there are no other requirements imposed (except of course referencing a destroyed or not yet initialized allocator is UB), in particular omp_alloc etc. can be passed omp_null_allocator, or a variable, and similarly for allocate clause etc. Without requires dynamic_allocators, there are various extra restrictions imposed: 1) omp_init_allocator/omp_destroy_allocator may not be called (except for implicit calls to it from uses_allocators) in a target region 2) omp_alloc etc. can't be called with omp_null_allocator and the argument has to be a constant expression for a predefined memory allocator (that is also mentioned on uses_allocators, though that doesn't have to be visible in the source because it could be lexically included in a target construct's body) 3) for allocate directive on static vars the above applies plus it has to be mentioned in uses_allocators 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 unclear if that was really intended. 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. 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 pointer 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. 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] = { ... }; #pragma omp target uses_allocators(h(t)) firstprivate(t) { } ok or not? We need to firstprivatize t so that we can call h = omp_init_allocator (omp_default_mem_space, 3, t); in the target region and it is kind of difficult to privatize the same var multiple times. And yet another issue, in omp_alloctrait_t one can point to other allocators (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. More on the actual patch later. Jakub