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.133.124]) by sourceware.org (Postfix) with ESMTPS id 26ACD385DC09 for ; Mon, 30 May 2022 17:24:04 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 26ACD385DC09 Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-612-D7Pp-YyfNIaHsnUGzp5Gzg-1; Mon, 30 May 2022 13:24:00 -0400 X-MC-Unique: D7Pp-YyfNIaHsnUGzp5Gzg-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 6FFD429DD99A; Mon, 30 May 2022 17:24:00 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.33.36.77]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 2EBF92026D07; Mon, 30 May 2022 17:23:59 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.17.1/8.17.1) with ESMTPS id 24UHNvgq3665234 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Mon, 30 May 2022 19:23:57 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.17.1/8.17.1/Submit) id 24UHNu7o3665233; Mon, 30 May 2022 19:23:56 +0200 Date: Mon, 30 May 2022 19:23:55 +0200 From: Jakub Jelinek To: Chung-Lin Tang Cc: Tobias Burnus , gcc-patches , Hafiz Abid Qadeer , Andrew Stubbs 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.78 on 10.11.54.4 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.3 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_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: Mon, 30 May 2022 17:24:07 -0000 On Mon, May 30, 2022 at 10:43:30PM +0800, Chung-Lin Tang wrote: > > This feels like you only accept a single allocator in the new syntax, > > but that isn't my reading of the spec, I'd understand it as: > > uses_allocators (memspace(omp_high_bw_mem_space), traits(foo_traits) : bar, baz, qux) > > being valid too. > > This patch now allows multiple allocators to be specified in new syntax, although I have > to note that the 5.2 specification of uses_allocators (page 181) specifically says > "allocator: expression of allocator_handle_type" for the "Arguments" description, > not a "list" like the allocate clause. I guess this should be raised on omp-lang then what we really want. Because the 5.1 syntax definitely allowed multiple allocators. > > It should be only removed if we emit the error (again with break; too). > > IMHO (see the other mail) we should complain here if it has value 0 > > (the omp_null_allocator case), dunno if we should error or just warn > > if the value is outside of the range of known predefined identifiers (1..8 > > currently I think). > But, otherwise, IMHO we need to keep it around, perhaps replacing the > > CONST_DECL with INTEGER_CST, for the purposes of checking what predefined > > allocators are used in the region. > > omp_alloc in libgomp does handle the omp_null_allocator case, by converting it > to something else. Sure, but the spec says that omp_alloc (42, omp_null_allocator) is invalid in target regions unless requires dynamic_allocators is seen first. And uses_allocators (omp_null_allocator) shouldn't make that valid. > @@ -15651,6 +15653,199 @@ c_parser_omp_clause_allocate (c_parser *parser, tree list) > return nl; > } > > +/* OpenMP 5.0: > + uses_allocators ( allocator-list ) > + > + allocator-list: > + allocator > + allocator , allocator-list > + allocator ( traits-array ) > + allocator ( traits-array ) , allocator-list > + > + OpenMP 5.2: > + > + uses_allocators ( modifier : allocator-list ) > + uses_allocators ( modifier , modifier : allocator-list ) > + > + modifier: > + traits ( traits-array ) > + memspace ( mem-space-handle ) */ > + > +static tree > +c_parser_omp_clause_uses_allocators (c_parser *parser, tree list) > +{ > + location_t clause_loc = c_parser_peek_token (parser)->location; > + tree t = NULL_TREE, nl = list; > + matching_parens parens; > + if (!parens.require_open (parser)) > + return list; > + > + tree memspace_expr = NULL_TREE; > + tree traits_var = NULL_TREE; > + > + struct item_tok > + { > + location_t loc; > + tree id; > + item_tok (void) : loc (UNKNOWN_LOCATION), id (NULL_TREE) {} > + }; > + struct item { item_tok name, arg; }; > + auto_vec *modifiers = NULL, *allocators = NULL; > + auto_vec *cur_list = new auto_vec (4); Each vec/auto_vec with a new type brings quite some overhead, a lot of functions need to be instantiated for it. I think it would be far easier to use raw token parsing: unsigned int pos = 1; bool has_modifiers = false; while (true) { c_token *tok = c_parser_peek_nth_token_raw (parser, pos); if (tok->type != CPP_NAME) break; ++pos; if (c_parser_peek_nth_token_raw (parser, pos + 1)->type == CPP_OPEN_PAREN) { ++pos; if (!c_parser_check_balanced_raw_token_sequence (parser, &pos) || c_parser_peek_nth_token_raw (parser, pos)->type != CPP_CLOSE_PAREN) break; ++pos; } tok = c_parser_peek_nth_token_raw (parser, pos); if (tok->type == CPP_COLON) { has_modifiers = true; break; } if (tok->type != CPP_COMMA) break; ++pos; } This should (haven't tested it though, so sorry if there are errors) cheaply determine if one should or shouldn't parse modifiers and then can just do parsing of modifiers if (has_modifiers) and then just the list (with ()s inside of list only if (!has_modifiers)). > @@ -21093,7 +21292,8 @@ c_parser_omp_target_exit_data (location_t loc, c_parser *parser, > | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_IN_REDUCTION) \ > | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_THREAD_LIMIT) \ > | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_IS_DEVICE_PTR)\ > - | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_HAS_DEVICE_ADDR)) > + | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_HAS_DEVICE_ADDR)\ Can you please fix up both the IS_DEVICE_PTR and newly added HAS_DEVICE_ADDR change to have a space before \ ? > + | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_USES_ALLOCATORS)) > +static tree > +cp_parser_omp_clause_uses_allocators (cp_parser *parser, tree list) > +{ > + location_t clause_loc > + = cp_lexer_peek_token (parser->lexer)->location; > + tree t = NULL_TREE, nl = list; > + matching_parens parens; > + if (!parens.require_open (parser)) > + return list; > + > + tree memspace_expr = NULL_TREE; > + tree traits_var = NULL_TREE; > + > + struct item_tok > + { > + location_t loc; > + tree id; > + item_tok (void) : loc (UNKNOWN_LOCATION), id (NULL_TREE) {} > + }; > + struct item { item_tok name, arg; }; > + auto_vec *modifiers = NULL, *allocators = NULL; > + auto_vec *cur_list = new auto_vec (4); In C++ FE one can use tentative parsing instead, but nth_token will work too. size_t pos = 1; bool has_modifiers = false; while (true) { if (!cp_lexer_nth_token_is (parser, pos, CPP_NAME)) break; ++pos; if (cp_lexer_nth_token_is (parser, pos, CPP_OPEN_PAREN)) { size_t npos = cp_parser_skip_balanced_tokens (parser, pos); if (npos == pos) break; pos = npos; } cp_token *tok = cp_lexer_peek_nth_token (parser, pos); if (tok->type == CPP_COLON) { has_modifiers = true; break; } if (tok->type != CPP_COMMA) break; ++pos; } > @@ -44464,7 +44684,8 @@ cp_parser_omp_target_update (cp_parser *parser, cp_token *pragma_tok, > | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_IN_REDUCTION) \ > | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_THREAD_LIMIT) \ > | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_IS_DEVICE_PTR)\ > - | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_HAS_DEVICE_ADDR)) > + | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_HAS_DEVICE_ADDR)\ Ditto. > + case OMP_CLAUSE_USES_ALLOCATORS: > + t = OMP_CLAUSE_USES_ALLOCATORS_ALLOCATOR (c); > + if (TREE_CODE (t) == FIELD_DECL) > + { > + sorry_at (OMP_CLAUSE_LOCATION (c), "class members not yet " > + "supported in % clause"); > + remove = true; > + break; > + } > + t = convert_from_reference (t); Are you sure about the above line? Should we allow omp_allocator_handle_t & type vars in the list? > + if (TREE_CODE (TREE_TYPE (t)) != ENUMERAL_TYPE > + || strcmp (IDENTIFIER_POINTER (TYPE_IDENTIFIER (TREE_TYPE (t))), > + "omp_allocator_handle_t") != 0) On the other side, if type_dependent_expression_p (t), I think we shouldn't diagnose this but postpone it till instantiation. And there should be in the testsuite a C++ testcase, where it uses_allocators inside of a template, in one place with a non-dependent omp_allocator_handle_t type, in another case e.g. with template parameter type that is later instantiated with omp_allocator_handle_t type. > + { > + error_at (OMP_CLAUSE_LOCATION (c), > + "allocator must be of % type"); > + remove = true; > + break; > + } > + if (TREE_CODE (t) == CONST_DECL) > + { > + /* Currently for pre-defined allocators in libgomp, we do not > + require additional init/fini inside target regions, so discard > + such clauses. */ > + remove = true; As I said earlier, I'd prefer to keep them and if for now you don't want to warn for uses of allocators that aren't mentioned in uses_allocators, just ignore them when actually privatizing them. > + > + if (OMP_CLAUSE_USES_ALLOCATORS_MEMSPACE (c) > + || OMP_CLAUSE_USES_ALLOCATORS_TRAITS (c)) > + { > + error_at (OMP_CLAUSE_LOCATION (c), > + "modifiers cannot be used with pre-defined " > + "allocators"); But of course this case should have remove = true; Jakub