Re: [PATCH 1/8] OpenMP: lvalue parsing for map/to/from clauses (C++)
On Wed, 10 Jan 2024 10:14:41 +0100 Jakub Jelinek wrote: > On Fri, Jan 05, 2024 at 12:23:26PM +, Julian Brown wrote: > > * g++.dg/gomp/bad-array-section-10.C: New test. > > This test FAILs in C++23/C++26 modes, just try > make check-g++ GXX_TESTSUITE_STDS=98,11,14,17,20,23,26 > RUNTESTFLAGS=gomp.exp=bad-array-section-10.C While in C++20 comma in > array references was deprecated, in C++23 we implement > multidimensional arrays, so the diagnostics there is different. See > https://wg21.link/p2036r3 Thanks -- I've pushed a patch to fix this. The bad-array-section-11.C test covered the C++23 case already, but I don't think normal testing iterates the newer language standards, hence missing this. Julian
Re: [PATCH 1/8] OpenMP: lvalue parsing for map/to/from clauses (C++)
On Fri, Jan 05, 2024 at 12:23:26PM +, Julian Brown wrote: > * g++.dg/gomp/bad-array-section-10.C: New test. This test FAILs in C++23/C++26 modes, just try make check-g++ GXX_TESTSUITE_STDS=98,11,14,17,20,23,26 RUNTESTFLAGS=gomp.exp=bad-array-section-10.C While in C++20 comma in array references was deprecated, in C++23 we implement multidimensional arrays, so the diagnostics there is different. See https://wg21.link/p2036r3 Jakub
Re: [PATCH 1/8] OpenMP: lvalue parsing for map/to/from clauses (C++)
Hi Julian! On 2024-01-07T16:04:37+0100, Tobias Burnus wrote: > Am 05.01.24 um 13:23 schrieb Julian Brown: >> Here's a rebased/retested version [...] > LGTM - [...] Got pushed as commit r14-7033-g1413af02d62182bc1e19698aaa4dae406f8f13bf "OpenMP: lvalue parsing for map/to/from clauses (C++)". Some (hopefully minor) tuning in the test cases is necessary; for example, for x86_64-pc-linux-gnu '-m32' testing, I see a few FAILs: +PASS: g++.dg/gomp/array-section-1.C -std=c++98 scan-tree-dump original "map\\(tofrom:arr1\\[1\\] [len: x != 0 ? [0-9]+ : [0-9]+\\]\\) map\\(firstprivate:arr1 \\[pointer assign, bias: [0-9]+\\]\\)" +PASS: g++.dg/gomp/array-section-1.C -std=c++98 scan-tree-dump original "map\\(tofrom:arr1\\[1\\] \\[len: x != 0 \\? [0-9]+ : [0-9]+\\]\\) map\\(firstprivate:arr1 \\[pointer assign, bias: [0-9]+\\]\\)" +FAIL: g++.dg/gomp/array-section-1.C -std=c++98 scan-tree-dump original "map\\(tofrom:arr1\\[SAVE_EXPR \\] \\[len: [0-9]+\\]\\) map\\(firstprivate:arr1 \\[pointer assign, bias: \\(long int\\) &arr1\\[SAVE_EXPR \\] - \\(long int\\) &arr1\\]\\)" +FAIL: g++.dg/gomp/array-section-1.C -std=c++98 scan-tree-dump original "map\\(tofrom:arr1\\[SAVE_EXPR \\] \\[len: [0-9]+\\]\\) map\\(firstprivate:arr1 \\[pointer assign, bias: \\(long int\\) &arr1\\[SAVE_EXPR \\] - \\(long int\\) &arr1\\]\\)" +FAIL: g++.dg/gomp/array-section-1.C -std=c++98 scan-tree-dump original "map\\(tofrom:arr1\\[SAVE_EXPR \\] \\[len: [0-9]+\\]\\) map\\(firstprivate:arr1 \\[pointer assign, bias: \\(long int\\) &arr1\\[SAVE_EXPR \\] - \\(long int\\) &arr1\\]\\)" +FAIL: g++.dg/gomp/array-section-1.C -std=c++98 scan-tree-dump original "map\\(tofrom:arr1\\[SAVE_EXPR \\] \\[len: [0-9]+\\]\\) map\\(firstprivate:arr1 \\[pointer assign, bias: \\(long int\\) &arr1\\[SAVE_EXPR \\] - \\(long int\\) &arr1\\]\\)" +PASS: g++.dg/gomp/array-section-1.C -std=c++98 (test for excess errors) Etc. +PASS: g++.dg/gomp/array-section-2.C -std=c++98 scan-tree-dump original "map\\(tofrom:arr1\\[0\\] \\[len: \\(sizetype\\) y \\* [0-9]+\\]\\) map\\(firstprivate:arr1 \\[pointer assign, bias: 0\\]\\)" +PASS: g++.dg/gomp/array-section-2.C -std=c++98 scan-tree-dump original "map\\(tofrom:arr1\\[0\\] \\[len: \\(sizetype\\) y \\* [0-9]+\\]\\) map\\(firstprivate:arr1 \\[pointer assign, bias: 0\\]\\)" +FAIL: g++.dg/gomp/array-section-2.C -std=c++98 scan-tree-dump original "map\\(tofrom:arr1\\[SAVE_EXPR \\] \\[len: \\(40 - \\(sizetype\\) SAVE_EXPR \\) \\* [0-9]+\\]\\) map\\(firstprivate:arr1 \\[pointer assign, bias: \\(long int\\) &arr1\\[SAVE_EXPR \\] - \\(long int\\) &arr1\\]\\)" +FAIL: g++.dg/gomp/array-section-2.C -std=c++98 scan-tree-dump original "map\\(tofrom:arr1\\[SAVE_EXPR \\] \\[len: \\(40 - \\(sizetype\\) SAVE_EXPR \\) \\* [0-9]+\\]\\) map\\(firstprivate:arr1 \\[pointer assign, bias: \\(long int\\) &arr1\\[SAVE_EXPR \\] - \\(long int\\) &arr1\\]\\)" +FAIL: g++.dg/gomp/array-section-2.C -std=c++98 scan-tree-dump original "map\\(tofrom:arr1\\[SAVE_EXPR \\] \\[len: \\(sizetype\\) y \\* [0-9]+\\]\\) map\\(firstprivate:arr1 \\[pointer assign, bias: \\(long int\\) &arr1\\[SAVE_EXPR \\] - \\(long int\\) &arr1\\]\\)" +FAIL: g++.dg/gomp/array-section-2.C -std=c++98 scan-tree-dump original "map\\(tofrom:arr1\\[SAVE_EXPR \\] \\[len: \\(sizetype\\) y \\* [0-9]+\\]\\) map\\(firstprivate:arr1 \\[pointer assign, bias: \\(long int\\) &arr1\\[SAVE_EXPR \\] - \\(long int\\) &arr1\\]\\)" +PASS: g++.dg/gomp/array-section-2.C -std=c++98 (test for excess errors) Etc. +PASS: g++.dg/gomp/bad-array-section-4.C -std=c++98 at line 15 (test for errors, line 14) +PASS: g++.dg/gomp/bad-array-section-4.C -std=c++98 at line 16 (test for errors, line 14) +PASS: g++.dg/gomp/bad-array-section-4.C -std=c++98 at line 17 (test for errors, line 14) +PASS: g++.dg/gomp/bad-array-section-4.C -std=c++98 at line 22 (test for warnings, line 21) +PASS: g++.dg/gomp/bad-array-section-4.C -std=c++98 at line 36 (test for errors, line 35) +FAIL: g++.dg/gomp/bad-array-section-4.C -std=c++98 at line 37 (test for warnings, line 35) +PASS: g++.dg/gomp/bad-array-section-4.C -std=c++98 at line 38 (test for errors, line 35) +PASS: g++.dg/gomp/bad-array-section-4.C -std=c++98 at line 39 (test for errors, line 35) +PASS: g++.dg/gomp/bad-array-section-4.C -std=c++98 at line 44 (test for warnings, line 43) +PASS: g++.dg/gomp/bad-array-section-4.C -std=c++98 (test for excess errors) Etc. Grüße Thomas - Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955
Re: [PATCH 1/8] OpenMP: lvalue parsing for map/to/from clauses (C++)
Am 05.01.24 um 13:23 schrieb Julian Brown:
On Wed, 20 Dec 2023 15:31:15 +0100
Tobias Burnus wrote:
Here's a rebased/retested version which fixes those bits (I haven't
adjusted the libgomp.texi bit you noted yet, though).
How does this look now?
--- a/gcc/gimplify.cc
+++ b/gcc/gimplify.cc
@@ -13499,7 +13499,11 @@ gimplify_adjust_omp_clauses_1 (splay_tree_node n, void
*data)
if (TREE_CODE (dtype) == REFERENCE_TYPE)
dtype = TREE_TYPE (dtype);
/* FIRSTPRIVATE_POINTER doesn't work well if we have a
-multiply-indirected pointer. */
+multiply-indirected pointer. If we have a reference to a pointer to
+a pointer, it's possible that this should really be
+GOMP_MAP_FIRSTPRIVATE_REFERENCE -- but that also doesn't work at the
+moment, so stick with this. (See testcase
+baseptrs-4.C:ref2ptrptr_offset_decl_member_slice). */
Looks as we should have a tracking PR about this; can you file one?
* * *
+ if (processing_template_decl)
+{
+ if (type_dependent_expression_p (array_expr)
+ || type_dependent_expression_p (index)
+ || type_dependent_expression_p (length))
+ return build_min_nt_loc (loc, OMP_ARRAY_SECTION, array_expr, index,
+length);
+}
I personally find it more readable if combined in a single 'if' condition.
+ /* Turn *foo into foo[0:1]. */
+ decl = TREE_OPERAND (decl, 0);
+ STRIP_NOPS (decl);
+
+ /* If we have "*foo" and
+- it's an indirection of a reference, "unconvert" it, i.e.
+ strip the indirection (to just "foo").
+- it's an indirection of a pointer, turn it into
+ "foo[0:1]". */
+ if (!ref_p)
+ decl = grok_omp_array_section (loc, decl, integer_zero_node,
+ integer_one_node);
I would remove the first comment and remove the two succeeding lines
below the second comment.
+ /* This code rewrites a parsed expression containing various tree
+codes used to represent array accesses into a more uniform nest of
+OMP_ARRAY_SECTION nodes before it is processed by
+semantics.cc:handle_omp_array_sections_1. It might be more
+efficient to move this logic to that function instead, analysing
+the parsed expression directly rather than this preprocessed
+form. */
Or to do this transformation in handle_omp_array_sections to get still a
unified result in the middle end. I see advantages of all three
solutions. (Doing this in parse.cc (as currently done) feels a bit odd,
though.)
* * *
build_omp_array_section (location_t loc, tree array_expr, tree index,
+tree length)
+{
+ tree idxtype;
+
+ /* 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 type = TREE_TYPE (array_expr);
+ gcc_assert (type);
+ type = non_reference (type);
+
+ tree sectype, eltype = TREE_TYPE (type);
+
+ /* 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 (array_expr);
+ else
+sectype = build_array_type (eltype, idxtype);
+
+ return build3_loc (loc, OMP_ARRAY_SECTION, sectype, array_expr, index,
+length);
+}
I wonder whether it would be more readable if one moves all the
'idxtype' handling into the last 'else' branch.
* * *
LGTM - please file the PR and consider the readability items above.
Thanks,
Tobias
Re: [PATCH 1/8] OpenMP: lvalue parsing for map/to/from clauses (C++)
On 05.09.23 21:28, Julian Brown wrote:
This patch supports "lvalue" parsing (or "locator list item type" parsing)
for several OpenMP clause types for C++, as required for OpenMP 5.0
and above. It is based on the version committed to the og13 branch,
posted here:
https://gcc.gnu.org/pipermail/gcc-patches/2023-June/623354.html
which in turn was based on the last version posted upstream:
https://gcc.gnu.org/pipermail/gcc-patches/2022-December/609040.html
This version has mostly just been rebased.
I had a first pass at the patch and didn't spot anything (C++ code wise);
I have some build/rebase issues and one .texi comment that is trivial and
could also be handled together with the 2/8 patch (adding C support).
* * *
Another rebase is required because of:
1 out of 22 hunks FAILED -- saving rejects to file gcc/cp/parser.cc.rej
1 out of 4 hunks FAILED -- saving rejects to file gcc/cp/pt.cc.rej
but those patch-apply fails look trivial to fix.
And during build, I see:
gcc/cp/decl2.cc:643:20: error: ‘build_non_dependent_expr’ was not declared in
this scope; did you mean ‘fold_non_dependent_expr’?
For details, see the two commits:
cd0e05b7ac3 c++: remove NON_DEPENDENT_EXPR, part 2
dad311874ac c++: remove NON_DEPENDENT_EXPR, part 1
* * *
When commenting those, even more build issues show up:
../../repos/gcc/gcc/cp/pt.cc:20493:20: error: ‘tsubst_copy’ was not declared in
this scope; did you mean ‘tsubst_scope’?
20493 | tree op0 = tsubst_copy (TREE_OPERAND (t, 0), args, complain,
in_decl);
|^~~
|tsubst_scope
In file included from ../../repos/gcc/gcc/cp/pt.cc:31:
../../repos/gcc/gcc/cp/pt.cc: In function ‘bool
value_dependent_expression_p(tree)’:
../../repos/gcc/gcc/cp/pt.cc:28009:41: error: ‘t’ was not declared in this
scope; did you mean ‘tm’?
28009 | tree op0 = RECUR (TREE_OPERAND (t, 0));
| ^
../../repos/gcc/gcc/system.h:1178:61: note: in definition of macro ‘CONST_CAST2’
1178 | #define CONST_CAST2(TOTYPE,FROMTYPE,X) (const_cast (X))
| ^
../../repos/gcc/gcc/tree.h:1285:31: note: in expansion of macro
‘TREE_OPERAND_CHECK’
1285 | #define TREE_OPERAND(NODE, I) TREE_OPERAND_CHECK (NODE, I)
| ^~
../../repos/gcc/gcc/cp/pt.cc:28009:27: note: in expansion of macro
‘TREE_OPERAND’
28009 | tree op0 = RECUR (TREE_OPERAND (t, 0));
| ^~~~
../../repos/gcc/gcc/cp/pt.cc:28009:20: error: ‘RECUR’ was not declared in this
scope
28009 | tree op0 = RECUR (TREE_OPERAND (t, 0));
|^
../../repos/gcc/gcc/cp/pt.cc:28012:11: error: ‘RETURN’ was not declared in this
scope
28012 | RETURN (error_mark_node);
| ^~
* * *
BTW: There is OpenMP specification Issue 2618 which is about code like "map(*p =
10)" with the intent to disallow it.
That's in line with the current code which prints a 'sorry' for those.
* * *
libgomp.texi contains:
@item C/C++'s lvalue expressions in @code{to}, @code{from}
and @code{map} clauses @tab N @tab
I think this can be set to 'P' + 'Only C++'; I think except for questionable
code like '*p = 10'
the support is complete, isn't it? If no, 'Initial support for C++, only' could
be a comment.
Alternatively, we can fold it into the next patch (which adds C support).
* * *
2023-09-05 Julian Brown
gcc/c-family/
* c-common.h (c_omp_address_inspector): Remove static from get_origin
and maybe_unconvert_ref methods.
* c-omp.cc (c_omp_split_clauses): Support OMP_ARRAY_SECTION.
(c_omp_address_inspector::map_supported_p): Handle OMP_ARRAY_SECTION.
(c_omp_address_inspector::get_origin): Avoid dereferencing possibly
NULL type when processing template decls.
(c_omp_address_inspector::maybe_unconvert_ref): Likewise.
gcc/cp/
* constexpr.cc (potential_consant_expression_1): Handle
OMP_ARRAY_SECTION.
* cp-tree.h (grok_omp_array_section, build_omp_array_section): Add
prototypes.
* decl2.cc (grok_omp_array_section): New function.
* 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. Support generalised lvalue parsing for
OpenMP map, to and from clauses. Use OMP_ARRAY_SECTION
code instead of TREE_LIST to represent OpenMP array sections.
(cp_parser_omp_var_list): Remove ALLOW_DEREF parameter, add M
