Hi Jakub, Thanks for the review! Here's a new version. On Wed, 2 Nov 2022 12:58:37 +0100 Jakub Jelinek via Fortran wrote: > > > 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. This patch uses OMP_ARRAY_SECTION for the previous parsing method also. (I've incorporated the followup "TREE_LIST->OMP_ARRAY_SECTION" for C++ patch into this one, since there probably isn't much point in keeping them separate.) > > @@ -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. I've changed these where surrounding code isn't using the old method, as suggested. > > + 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. I've changed that. > > + > > + 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. This version of the patch splits OMP_ARRAY_SECTION node creation into two new functions in decl2.cc and semantics.cc, approximately mimicking ARRAY_REF handling -- which works, though I'm not sure if the separation of responsibilities is perfect in our case. > 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. I've augmented some of the tests with template bits and added a couple of new ones also. > 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 :. ...and I've added an error for attempts to use multidimensional arrays in C++23, and a deprecation warning for "," in OMP array sections for C++20 (see new tests). I've also added tests for some potentially-awkward parsing ambiguities, namely the ternary operator and the scope-resolution operator. E.g. for the latter we can do: map(myarray[::x : ::y]) which is unfortunately ambiguous if we omit the spaces because the triple colon can be interpreted in two ways (a little like the famous ">>" double template close vs. right-shift ambiguity). How does this version look? Re-tested with offloading to NVPTX and bootstrapped (with "OpenMP/OpenACC: Rework clause expansion and nested struct handling" also applied). Thanks, Julian