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 9155D396DC14 for ; Thu, 19 May 2022 17:47:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 9155D396DC14 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-107-faHDkpIOP1aIMzeryqre0Q-1; Thu, 19 May 2022 13:47:00 -0400 X-MC-Unique: faHDkpIOP1aIMzeryqre0Q-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 65C69100DE72; Thu, 19 May 2022 17:47:00 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.192.23]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 250D71121314; Thu, 19 May 2022 17:46:59 +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 24JHkvfV077388 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Thu, 19 May 2022 19:46:57 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.16.1/8.16.1/Submit) id 24JHkteX077387; Thu, 19 May 2022 19:46:55 +0200 Date: Thu, 19 May 2022 19:46:55 +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.78 on 10.11.54.3 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 17:47:04 -0000 On Tue, May 10, 2022 at 07:29:23PM +0800, Chung-Lin Tang wrote: > @@ -15624,6 +15626,233 @@ c_parser_omp_clause_allocate (c_parser *parser, tree list) > return nl; > } > > +/* OpenMP 5.2: > + uses_allocators ( allocator-list ) As uses_allocators is a 5.0 feature already, the above should say /* OpenMP 5.0: > + > + allocator-list: > + allocator > + allocator , allocator-list > + allocator ( traits-array ) > + allocator ( traits-array ) , allocator-list > + And here it should add OpenMP 5.2: > + uses_allocators ( modifier : allocator ) > + uses_allocators ( modifier , modifier : allocator ) > + > + 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; > + > + bool has_modifiers = false; > + tree memspace_expr = NULL_TREE; > + tree traits_var = NULL_TREE; > + > + if (c_parser_next_token_is (parser, CPP_NAME)) > + { > + c_token *tok = c_parser_peek_token (parser); > + const char *p = IDENTIFIER_POINTER (tok->value); > + > + if (strcmp ("traits", p) == 0 || strcmp ("memspace", p) == 0) > + { > + has_modifiers = true; > + c_parser_consume_token (parser); > + matching_parens parens2;; Double ;;, should be just ; But more importantly, it is more complex. When you see uses_allocators(traits or uses_allocators(memspace it is not given that it has modifiers. While the 5.0/5.1 syntax had a restriction that when allocator is not a predefined allocator (and traits or memspace aren't predefined allocators) it must use ()s with traits, so uses_allocators(traits) uses_allocators(memspace) uses_allocators(traits,memspace) are all invalid, omp_allocator_handle_t traits; uses_allocators(traits(mytraits)) or omp_allocator_handle_t memspace; uses_allocators(memspace(mytraits),omp_default_mem_alloc) are valid in the old syntax. So, I'm afraid to find out if the traits or memspace identifier seen after uses_allocator ( are modifiers or not we need to peek (for C with c_parser_peek_nth_token_raw) through all the modifiers whether we see a : and only in that case say they are modifiers rather than the old style syntax. > + parens2.require_open (parser); > + > + if (c_parser_next_token_is (parser, CPP_NAME) > + && (c_parser_peek_token (parser)->id_kind == C_ID_ID > + || c_parser_peek_token (parser)->id_kind == C_ID_TYPENAME)) > + { > + tok = c_parser_peek_token (parser); > + t = lookup_name (tok->value); > + > + if (t == NULL_TREE) > + { > + undeclared_variable (tok->location, tok->value); > + t = error_mark_node; > + } > + else > + { > + if (strcmp ("memspace", p) == 0) I think it would be better to have bool variable whether it was memspace or traits modifier, so strcmp just once with each string, not multiple times. In the 5.2 syntax, for memspace it is clear that it has to be an identifier that is the predefined namespace, but for traits it just says it is a variable of alloctrait array type, it is unclear if it must be an identifier or could be say structure element or array element etc. Guess something to discuss on omp-lang and for now pretend it must be an identifier. > + if (c_parser_next_token_is (parser, CPP_COMMA)) > + { > + c_parser_consume_token (parser); > + tok = c_parser_peek_token (parser); > + const char *q = ""; > + if (c_parser_next_token_is (parser, CPP_NAME)) > + q = IDENTIFIER_POINTER (tok->value); > + if (strcmp (q, "memspace") != 0 && strcmp (q, "traits") != 0) > + { > + c_parser_error (parser, "expected % or %"); > + parens.skip_until_found_close (parser); > + return list; > + } I don't really like the modifiers handling not done in a loop. As I said above, there needs to be some check whether there are modifiers or not, but once we figure out there are modifiers, it should be done in a loop with say some mask var on which traits have been already handled to diagnose duplicates, we don't want to do the parsing code twice. > + if (has_modifiers) > + { > + if (!c_parser_require (parser, CPP_COLON, "expected %<:%>")) > + { > + parens.skip_until_found_close (parser); > + return list; > + } > + > + if (c_parser_next_token_is (parser, CPP_NAME) > + && c_parser_peek_token (parser)->id_kind == C_ID_ID) > + { > + tree t = lookup_name (c_parser_peek_token (parser)->value); > + > + if (t == NULL_TREE) > + { > + undeclared_variable (c_parser_peek_token (parser)->location, > + c_parser_peek_token (parser)->value); > + t = error_mark_node; > + } > + else if (t != error_mark_node) > + { > + tree c = build_omp_clause (clause_loc, > + OMP_CLAUSE_USES_ALLOCATORS); > + OMP_CLAUSE_USES_ALLOCATORS_ALLOCATOR (c) = t; > + OMP_CLAUSE_USES_ALLOCATORS_MEMSPACE (c) = memspace_expr; > + OMP_CLAUSE_USES_ALLOCATORS_TRAITS (c) = traits_var; > + OMP_CLAUSE_CHAIN (c) = list; > + > + nl = c; > + } > + c_parser_consume_token (parser); > + > + if (c_parser_next_token_is (parser, CPP_OPEN_PAREN)) > + c_parser_error (parser, "modifiers cannot be used with " > + "legacy array syntax"); > + else if (c_parser_next_token_is (parser, CPP_COMMA)) > + c_parser_error (parser, "modifiers can only be used with " > + "a single allocator in % " > + "clause"); > + } > + else > + c_parser_error (parser, "expected identifier"); 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. And, I'd strongly prefer to just c_parser_omp_variable_list at this point for the rest if there were modifiers and just fill in OMP_CLAUSE_USES_ALLOCATORS_MEMSPACE and OMP_CLAUSE_USES_ALLOCATORS_TRAITS on each similarly to how e.g. linear or other clause with modifiers are handled. The no modifiers case of course needs its own code so that it handles the ()s. > + case OMP_CLAUSE_USES_ALLOCATORS: > + t = OMP_CLAUSE_USES_ALLOCATORS_ALLOCATOR (c); > + if (bitmap_bit_p (&generic_head, DECL_UID (t)) > + || bitmap_bit_p (&map_head, DECL_UID (t)) > + || bitmap_bit_p (&firstprivate_head, DECL_UID (t)) > + || bitmap_bit_p (&lastprivate_head, DECL_UID (t))) You can't just use DECL_UID before you actually verify it is a variable. So IMHO this particular if should be moved down somewhat. > + { > + error_at (OMP_CLAUSE_LOCATION (c), > + "%qE appears more than once in data clauses", t); > + remove = true; > + } > + else > + bitmap_set_bit (&generic_head, DECL_UID (t)); > + if (TREE_CODE (TREE_TYPE (t)) != ENUMERAL_TYPE > + || strcmp (IDENTIFIER_POINTER (TYPE_IDENTIFIER (TREE_TYPE (t))), > + "omp_allocator_handle_t") != 0) > + { > + error_at (OMP_CLAUSE_LOCATION (c), > + "allocator must be of % type"); > + remove = true; > + } I'd add break; after remove = true; > + if (TREE_CODE (t) == CONST_DECL) > + { > + 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"); > + > + /* Currently for pre-defined allocators in libgomp, we do not > + require additional init/fini inside target regions, so discard > + such clauses. */ > + remove = true; > + } 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. But break; afterwards to avoid the code below. Then, do a VAR_DECL/PARM_DECL check, complain if it is not (see other spots in the function that do check that). Then the bitmap_bit_p stuff above. > + t = OMP_CLAUSE_USES_ALLOCATORS_MEMSPACE (c); > + if (t != NULL_TREE > + && (TREE_CODE (t) != CONST_DECL > + || TREE_CODE (TREE_TYPE (t)) != ENUMERAL_TYPE > + || strcmp (IDENTIFIER_POINTER (TYPE_IDENTIFIER (TREE_TYPE (t))), > + "omp_memspace_handle_t") != 0)) > + { > + error_at (OMP_CLAUSE_LOCATION (c), "memspace modifier must be " > + "constant enum of % type"); > + remove = true; > + } Again, wonder if it shouldn't after checking it replace the CONST_DECL with an INTEGER_CST for the purposes of the middle-end. > + t = OMP_CLAUSE_USES_ALLOCATORS_TRAITS (c); > + if (t != NULL_TREE) > + { > + bool type_err = false; > + > + if (TREE_CODE (TREE_TYPE (t)) != ARRAY_TYPE) > + type_err = true; > + else > + { > + tree elem_t = TREE_TYPE (TREE_TYPE (t)); > + if (TREE_CODE (elem_t) != RECORD_TYPE > + || strcmp (IDENTIFIER_POINTER (TYPE_IDENTIFIER (elem_t)), > + "omp_alloctrait_t") != 0 > + || !TYPE_READONLY (elem_t)) I'd diagnose if the array is incomplete, say extern omp_alloctrait_t traits[]; For the 5.2 syntax, there is also the restriction that "be defined in the same scope as the construct on which the clause appears" which I don't see being checked here. Unclear whether it applies to the old syntax too. But again, it should also check that it is a VAR_DECL, it isn't extern etc. For C++, I have to wonder if at allocator couldn't be a non-static data member of a class inside of methods, that is something that can be generally privatized too. Jakub