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 79A103858C00 for ; Wed, 2 Nov 2022 11:58:46 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 79A103858C00 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1667390326; h=from:from:reply-to:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:in-reply-to:in-reply-to: references:references; bh=Q9KwdD8z2w7pP1VUSDrW3vEl5rCG8oavqQ3VNQOjC/g=; b=D6a7x8iaoab0IWOQV6yd9OWtgUgiluusAWhRhkMJdwo6qM34x3vAnkd5YR1bgvtHKhC924 tAYlxeUgVpgZYl1gzaH1yEYJeQpbKMtowoHm8SVCrHTITFLyBJxlyL9USD/Grjfgs56yRH OqpVQn0PUVAnFI/ocp7yjkB2MVk9fcA= 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-481-qM8i1pslMUizcx7Pl1X-hw-1; Wed, 02 Nov 2022 07:58:42 -0400 X-MC-Unique: qM8i1pslMUizcx7Pl1X-hw-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.rdu2.redhat.com [10.11.54.7]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 74C8A811E75; Wed, 2 Nov 2022 11:58:42 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.193.252]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 12258141511F; Wed, 2 Nov 2022 11:58:41 +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 2A2BwdXP2092276 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Wed, 2 Nov 2022 12:58:39 +0100 Received: (from jakub@localhost) by tucnak.zalov.cz (8.17.1/8.17.1/Submit) id 2A2BwbC92092275; Wed, 2 Nov 2022 12:58:37 +0100 Date: Wed, 2 Nov 2022 12:58:37 +0100 From: Jakub Jelinek To: Julian Brown Cc: Jakub Jelinek via Fortran , Tobias Burnus , gcc-patches@gcc.gnu.org, Thomas Schwinge Subject: Re: [PATCH v2 06/11] OpenMP: lvalue parsing for map clauses (C++) Message-ID: Reply-To: Jakub Jelinek References: <62e4e371468638d2f155c528a5c1e597558a56ac.1647619144.git.julian@codesourcery.com> <20221101215038.08a688e1@squid.athome> MIME-Version: 1.0 In-Reply-To: <20221101215038.08a688e1@squid.athome> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.7 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.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Hi! Thanks for working on this! On Tue, Nov 01, 2022 at 09:50:38PM +0000, Julian Brown wrote: > > I think we should figure out when we should temporarily disable > > parser->omp_array_section_p = false; > > and restore it afterwards to a saved value. E.g. > > cp_parser_lambda_expression seems like a good candidate, the fact that > > OpenMP array sections are allowed say in map clause doesn't mean they > > are allowed inside of lambdas and it would be especially hard when > > the lambda is defining a separate function and the search for > > OMP_ARRAY_SECTION probably wouldn't be able to discover those. > > Other spots to consider might be statement expressions, perhaps type > > definitions etc. > > I've had a go at doing this -- several expression types now forbid > array-section syntax (see new "bad-array-section-*" tests added). I'm > afraid my C++ isn't quite up to figuring out how it's possible to > define a type inside an expression (inside a map clause) if we forbid > lambdas and statement expressions though -- can you give an example? But we can't forbid lambdas inside of the map clause expressions, they are certainly valid in OpenMP, and IMNSHO shouldn't disallow statement expressions, people might not even know they use a statement expression, they could just use some standard macro which uses a statement expression under the hood. Though your testcases look good. > > This shouldn't be done just for OMP_CLAUSE_MAP, but for all the > > other clauses that accept array sections, including > > OMP_CLAUSE_DEPEND, OMP_CLAUSE_AFFINITY, OMP_CLAUSE_MAP, OMP_CLAUSE_TO, > > OMP_CLAUSE_FROM, OMP_CLAUSE_INCLUSIVE, OMP_CLAUSE_EXCLUSIVE, > > OMP_CLAUSE_USE_DEVICE_ADDR, OMP_CLAUSE_HAS_DEVICE_ADDR, > > OMP_CLAUSE_*REDUCTION. > > I'm not too sure about all of those -- Tobias points out that > "INCLUSIVE", "EXCLUSIVE", *DEVICE* and *REDUCTION* take "variable list" > item types, not "locator list", though sometimes with an array section > being permitted (in OpenMP 5.2+). That is true. For the clauses that don't use locator lists but variable lists but accept array sections there are strict restrictions on what one can use, basically one can only have varname or varname[...] or varname[...][...] etc. where ... is the normal array element or array section syntax. So, we probably should continue to parse them as now, but we can use OMP_ARRAY_SECTION to hold what we've parsed or even share code with parsing array sections and the [...] on those clauses. > Tested (alongside next patch) with offloading to NVPTX -- with my > previously-posted "address tokenization" patch also applied. > 2022-11-01 Julian Brown > > gcc/c-family/ > * c-omp.cc (c_omp_address_inspector::map_supported_p): Handle > OMP_ARRAY_SECTION. > > gcc/cp/ > * constexpr.cc (potential_consant_expression_1): Handle > OMP_ARRAY_SECTION. > * error.cc (dump_expr): Handle OMP_ARRAY_SECTION. > * parser.cc (cp_parser_new): Initialize parser->omp_array_section_p. > (cp_parser_statement_expr): Disallow array sections. > (cp_parser_postfix_open_square_expression): Support OMP_ARRAY_SECTION > parsing. > (cp_parser_parenthesized_expression_list, cp_parser_lambda_expression, > cp_parser_braced_list): Disallow array sections. > (cp_parser_omp_var_list_no_open): Remove ALLOW_DEREF parameter, add > MAP_LVALUE in its place. Supported generalised lvalue parsing for > OpenMP map, to and from clauses. > (cp_parser_omp_var_list): Remove ALLOW_DEREF parameter, add MAP_LVALUE. > Pass to cp_parser_omp_var_list_no_open. > (cp_parser_oacc_data_clause, cp_parser_omp_all_clauses): Update calls > to cp_parser_omp_var_list. > (cp_parser_omp_clause_map): Add sk_omp scope around > cp_parser_omp_var_list_no_open call. > * parser.h (cp_parser): Add omp_array_section_p field. > * semantics.cc (handle_omp_array_sections_1): Handle more types of map > expression. > (handle_omp_array_section): Handle non-DECL_P attachment points. > (finish_omp_clauses): Check for supported types of expression. > > gcc/ > * tree-pretty-print.c (dump_generic_node): Support OMP_ARRAY_SECTION. > * tree.def (OMP_ARRAY_SECTION): New tree code. > > gcc/testsuite/ > * c-c++-common/gomp/map-6.c: Update expected output. > * g++.dg/gomp/bad-array-section-1.C: New test. > * g++.dg/gomp/bad-array-section-2.C: New test. > * g++.dg/gomp/bad-array-section-3.C: New test. > * g++.dg/gomp/bad-array-section-4.C: New test. > * g++.dg/gomp/bad-array-section-5.C: New test. > * g++.dg/gomp/bad-array-section-6.C: New test. > * g++.dg/gomp/bad-array-section-7.C: New test. > * g++.dg/gomp/bad-array-section-8.C: New test. > * g++.dg/gomp/bad-array-section-9.C: New test. > * g++.dg/gomp/has_device_addr-non-lvalue-1.C: New test. > * g++.dg/gomp/pr67522.C: Update expected output. > * g++.dg/gomp/ind-base-3.C: New test. > * g++.dg/gomp/map-assignment-1.C: New test. > * g++.dg/gomp/map-inc-1.C: New test. > * g++.dg/gomp/map-lvalue-ref-1.C: New test. > * g++.dg/gomp/map-ptrmem-1.C: New test. > * g++.dg/gomp/map-ptrmem-2.C: New test. > * g++.dg/gomp/map-static-cast-lvalue-1.C: New test. > * g++.dg/gomp/map-ternary-1.C: New test. > * g++.dg/gomp/member-array-2.C: New test. > > libgomp/ > * testsuite/libgomp.c++/baseptrs-4.C: Remove commented-out cases that > now work. > * testsuite/libgomp.c++/ind-base-1.C: New test. > * testsuite/libgomp.c++/ind-base-2.C: New test. > * testsuite/libgomp.c++/lvalue-tofrom-1.C: New test. > * testsuite/libgomp.c++/lvalue-tofrom-2.C: New test. > * testsuite/libgomp.c++/map-comma-1.C: New test. > * testsuite/libgomp.c++/map-rvalue-ref-1.C: New test. > * testsuite/libgomp.c++/member-array-1.C: New test. > * testsuite/libgomp.c++/struct-ref-1.C: New test. > * testsuite/libgomp.c++/array-field-1.C: New test. > * testsuite/libgomp.c++/array-of-struct-1.C: New test. > * testsuite/libgomp.c++/array-of-struct-2.C: New test. Some lines are (correctly) tab indented above, but others 8 spaces. This will not get through pre-commit hook. > @@ -5265,6 +5268,9 @@ static cp_expr > cp_parser_statement_expr (cp_parser *parser) > { > cp_token_position start = cp_parser_start_tentative_firewall (parser); > + bool saved_omp_array_section_p = parser->omp_array_section_p; > + > + parser->omp_array_section_p = false; The modern C++ FE way of doing this is auto oas = make_temp_override (parser->omp_array_section_p, false); where you don't need to do anything at the end and it handles say return in the middle or goto out of the block. It isn't appropriate when you need to restore it at some other location than the end of the containing block. > + if ((index && error_operand_p (index)) > + || (length && error_operand_p (length))) > + return error_mark_node; error_operand_p (NULL) works and returns false, so you can just do if (error_operand_p (index) || error_operand_p (length)) return error_mark_node; Though I wonder if the > + > + cp_parser_require (parser, CPP_CLOSE_SQUARE, RT_CLOSE_SQUARE); shouldn't be done before it, if one of the expressions is erroneous, but we have there ] after it, I think we want to continue parsing after it. > + > + tree idxtype; > + > + if (index) > + index = maybe_constant_value (index); > + if (length) > + length = maybe_constant_value (length); This is incorrect in templates (when processing_template_decl). maybe_constant_value should be only called when !processing_template_decl, above you should call fold_non_dependent_expr instead. And those handle (pass through) NULL_TREE, so no need to conditionalize. Or maybe even better maybe_fold_non_dependent_expr which will only return different tree from the original if it actually managed to fold it into a constant. Another thing is that in the C++ FE we usually do just parsing in parser.cc and the actual building of the expressions in some routine in semantics.cc or decl2.cc or so. The point is that it sometimes needs to be called twice, once during parsing (where for !processing_template_decl we handle everything right away, otherwise for processing_template_decl if the expressions (in your case postfix_expression, index and/or length) are type_dependent_expression_p, one doesn't do much processing and just build_min_nt_loc or so some (dependent) tree just to hold the expressions (OMP_ARRAY_SECTION in your case). Or, if processing_template_decl but nothing is type dependent, it does some diagnostics etc. with build_non_dependent_expr around the expressions, only to throw it away at the end unless it resulted in an error and still build a tree with the original expressions (but perhaps a non-dependent type of the whole expression). And then there needs to be a pt.cc case which handles the raw OMP_ARRAY_SECTION tree, will recurse on the subexpressions and finally call the function that is also called during the parsing to build the tree, but this time usually with !processing_template_decl, so it builds the final thing. For the normal array element parsing, this is done in grok_array_decl. In C++ test coverage, it is usually needed to have normal non-template tests (those can be often shared with C FE too in c-c++-common/gomp/), and then some tests in templates, and there both the non-dependent stuff, say if you write template void foo () { // normal C/C++ subset stuff here } void bar () { foo<0> (); } and then dependent stuff, where you have say template void baz (T x, U y, V z) { ... map(x[y:z]) } and the like or say value dependent N from the earlier template etc. to test that it works properly if some or all expressions are type or value dependent during parsing and only later on are instantiated. Yet another thing is that C++23 allows and we handle multidimensional subscript operator, see https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p2128r6.pdf Right now the multi-dimensional subscript only applies to cases where there is a user operator[] and I believe OpenMP array sections on the other side are strictly for cases where overloaded operators do not apply, but one can certainly write: #pragma omp target map(tofrom:a[1,2,3:4:5]) and while in C++20 and older that is parsed as a[(1,2,3):4:5] perhaps with a -Wcomma-subscript warning, in C++23 parsing that yields a vector of expressions. So, probably we need to diagnose the case where there is vector of expressions (i.e. 2 or more) as an error. C++23 also allows a[] but in that case one can't use the OpenMP array section syntax, it is handled only if [ is immediately followed by ] so there is no :. > + > + /* If we know the integer bounds, create an index type with exact > + low/high (or zero/length) bounds. Otherwise, create an incomplete > + array type. (This mostly only affects diagnostics.) */ > + if (index != NULL_TREE > + && length != NULL_TREE > + && TREE_CODE (index) == INTEGER_CST > + && TREE_CODE (length) == INTEGER_CST) > + { > + tree low = fold_convert (sizetype, index); > + tree high = fold_convert (sizetype, length); > + high = size_binop (PLUS_EXPR, low, high); > + high = size_binop (MINUS_EXPR, high, size_one_node); > + idxtype = build_range_type (sizetype, low, high); > + } > + else if ((index == NULL_TREE || integer_zerop (index)) > + && length != NULL_TREE > + && TREE_CODE (length) == INTEGER_CST) > + idxtype = build_index_type (length); > + else > + idxtype = NULL_TREE; > + > + tree eltype = ((postfix_expression != error_mark_node > + && TREE_TYPE (postfix_expression)) > + ? TREE_TYPE (TREE_TYPE (postfix_expression)) > + : NULL_TREE); > + > + tree sectype; > + > + /* It's not an array or pointer type. Just reuse the type of the > + original expression as the type of the array section (an error will be > + raised anyway, later). */ > + if (eltype == NULL_TREE) > + sectype = TREE_TYPE (postfix_expression); > + else > + sectype = build_array_type (eltype, idxtype); > + > + return build3_loc (input_location, OMP_ARRAY_SECTION, > + sectype, postfix_expression, index, length); > + } > + > + parser->colon_corrects_to_scope_p = saved_colon_corrects_to_scope_p; > + > /* Look for the closing `]'. */ > cp_parser_require (parser, CPP_CLOSE_SQUARE, RT_CLOSE_SQUARE); > > @@ -8484,6 +8563,7 @@ cp_parser_parenthesized_expression_list (cp_parser* parser, > { > vec *expression_list; > bool saved_greater_than_is_operator_p; > + bool saved_omp_array_section_p; > > /* Assume all the expressions will be constant. */ > if (non_constant_p) > @@ -8501,6 +8581,9 @@ cp_parser_parenthesized_expression_list (cp_parser* parser, > = parser->greater_than_is_operator_p; > parser->greater_than_is_operator_p = true; > > + saved_omp_array_section_p = parser->omp_array_section_p; > + parser->omp_array_section_p = false; Here the surrounding code uses the old style save/restore, so perhaps it is ok as is. > + > cp_expr expr (NULL_TREE); > > /* Consume expressions until there are no more. */ > @@ -8571,6 +8654,7 @@ cp_parser_parenthesized_expression_list (cp_parser* parser, > > parser->greater_than_is_operator_p > = saved_greater_than_is_operator_p; > + parser->omp_array_section_p = saved_omp_array_section_p; > > return expression_list; > } > @@ -11051,6 +11135,7 @@ cp_parser_lambda_expression (cp_parser* parser) > cp_binding_level* implicit_template_scope = parser->implicit_template_scope; > bool auto_is_implicit_function_template_parm_p > = parser->auto_is_implicit_function_template_parm_p; > + bool saved_omp_array_section_p = parser->omp_array_section_p; > > parser->num_template_parameter_lists = 0; > parser->in_statement = 0; > @@ -11059,6 +11144,7 @@ cp_parser_lambda_expression (cp_parser* parser) > parser->implicit_template_parms = 0; > parser->implicit_template_scope = 0; > parser->auto_is_implicit_function_template_parm_p = false; > + parser->omp_array_section_p = false; > > /* The body of a lambda in a discarded statement is not discarded. */ > bool discarded = in_discarded_stmt; > @@ -11111,6 +11197,7 @@ cp_parser_lambda_expression (cp_parser* parser) > parser->implicit_template_scope = implicit_template_scope; > parser->auto_is_implicit_function_template_parm_p > = auto_is_implicit_function_template_parm_p; > + parser->omp_array_section_p = saved_omp_array_section_p; > } Here too. > > /* This field is only used during parsing of the lambda. */ > @@ -25359,6 +25446,9 @@ cp_parser_braced_list (cp_parser* parser, bool* non_constant_p) > { > tree initializer; > location_t start_loc = cp_lexer_peek_token (parser->lexer)->location; > + bool saved_omp_array_section_p = parser->omp_array_section_p; > + > + parser->omp_array_section_p = false; But this could use make_temp_override. > + /* This condition doesn't include OMP_CLAUSE_DEPEND or > + OMP_CLAUSE_AFFINITY since lvalue ("locator list") parsing for those is > + handled further down the function. */ > + else if (map_lvalue > + && (kind == OMP_CLAUSE_MAP > + || kind == OMP_CLAUSE_TO > + || kind == OMP_CLAUSE_FROM)) > + { > + auto s = make_temp_override (parser->omp_array_section_p, true); You use make_temp_override already here ;) Jakub