Re: [PATCH v7 5/5] OpenMP/OpenACC: Reorganise OMP map clause handling in gimplify.cc
Hi Tobias,
Thanks for review! Here's a new version of the patch which hopefully
addresses this round of comments.
On Tue, 19 Dec 2023 16:41:54 +0100
Tobias Burnus wrote:
> On 16.12.23 14:25, Julian Brown wrote:
> > --- a/gcc/gimplify.cc
> > +++ b/gcc/gimplify.cc
> > @@ -10107,6 +10114,20 @@ omp_segregate_mapping_groups
> > (omp_mapping_group *inlist) ard_tail = >next;
> > break;
> >
> > + case GOMP_MAP_PRESENT_ALLOC:
> > + *pa_tail = w;
> > + w->next = NULL;
> > + pa_tail = >next;
> > + break;
> > +
> > + case GOMP_MAP_PRESENT_FROM:
> > + case GOMP_MAP_PRESENT_TO:
> > + case GOMP_MAP_PRESENT_TOFROM:
> > + *ptf_tail = w;
> > + w->next = NULL;
> > + ptf_tail = >next;
> > + break;
> > +
>
> First, I note that GOMP_MAP_PRESENT_ALLOC and
> GOMP_MAP_PRESENT_{FROM,TO,TOFROM} are semantically identical: If the
> variable is not present, error termination will happen - otherwise, if
> present, no data movement will happen. Hence, they will be changed to
> GOMP_MAP_FORCE_PRESENT in gimplify_adjust_omp_clauses.
>
> That's also the reason that the old code handled all of them
> identical.
>
> However, besides a plain 'present', there is also 'always' +
> 'present'. Those are different as after a normal 'present' check
> (abort if not present), the data copying will happen:
> GOMP_MAP_ALWAYS_PRESENT_TO, GOMP_MAP_ALWAYS_PRESENT_FROM,
> GOMP_MAP_ALWAYS_PRESENT_TOFROM.
>
> (Note that: always + present + alloc = GOMP_MAP_PRESENT_ALLOC (w/o
> 'always') as already done in the FE.)
>
> Thus, all 'case' from your patch should go to a single group (possibly
> adding a comment about it). The question is what to do with the
> 'present,always' case. I think leaving them under 'default:' is fine,
> but I might have missed something.
I've made this change (i.e.: grouping all "GOMP_MAP_PRESENT_*" nodes
together), and in fact that restores the dump output for the
gfortran.dg/gomp/map-12.f90 that needed to be adjusted for the previous
version of the patch (so that hunk has now disappeared).
> > default:
> > *tf_tail = w;
> > w->next = NULL;
> > @@ -10118,8 +10139,10 @@ omp_segregate_mapping_groups
> > (omp_mapping_group *inlist)
> * * *
> > @@ -11922,119 +11945,30 @@ gimplify_scan_omp_clauses (tree *list_p,
> > gimple_seq *pre_p, break;
> > }
> >
> > - if (code == OMP_TARGET
> > - || code == OMP_TARGET_DATA
> > - || code == OMP_TARGET_ENTER_DATA
> > - || code == OMP_TARGET_EXIT_DATA)
> > -{
> > - vec *groups;
> > - groups = omp_gather_mapping_groups (list_p);
> > - if (groups)
> > - {
> > - hash_map
> > *grpmap;
> > - grpmap = omp_index_mapping_groups (groups);
> > + vec *groups = omp_gather_mapping_groups
> > (list_p);
> > + hash_map *grpmap =
> > NULL;
> > + unsigned grpnum = 0;
> > + tree *grp_start_p = NULL, grp_end = NULL_TREE;
>
> ...
>
> > - else if (region_type & ORT_ACC)
> > -{
> I wonder whether you should not better call
> 'omp_gather_mapping_groups' only for the 'code == OMP_TARGET...' and
> for ORT_ACC (or some subset of OACC *), given that this function is
> also called bygimplify_omp_parallel, gimplify_omp_task,
> gimplify_omp_for, ...
>
> This avoids some memory allocation and list_p walking, i.e. it is not
> too bad - but also not really needed for task, parallel, for, ...
I've made that change -- OpenACC uses OMP_CLAUSE_MAP in quite a wide
range of directives, but the new version of the patch lists them
individually anyway, rather than using a catch-all for ORT_ACC regions.
That seems OK, I think.
> > @@ -14008,26 +13926,73 @@ gimplify_adjust_omp_clauses (gimple_seq
> > *pre_p, gimple_seq body, tree *list_p, default:
> > break;
> > }
> > - if (code == OMP_TARGET_EXIT_DATA
> > - && OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ALWAYS_POINTER)
> > + switch (code)
> > {
> > + case OMP_TARGET:
> > + break;
> > + case OACC_DATA:
> > + if (TREE_CODE (TREE_TYPE (decl)) != ARRAY_TYPE)
> > + break;
> > + goto check_firstprivate;
> > + case OACC_ENTER_DATA:
> > + case OACC_EXIT_DATA:
> > + case OMP_TARGET_DATA:
> > + case OMP_TARGET_ENTER_DATA:
> > + case OMP_TARGET_EXIT_DATA:
> > + case OACC_HOST_DATA:
> > + check_firstprivate:
> > + if (OMP_CLAUSE_MAP_KIND (c) ==
> > GOMP_MAP_FIRSTPRIVATE_POINTER
>
> I think it looks nicer if the OACC_HOST is before OMP_* such that all
> OACC_* are together. (In the old code, oacc_enter/exit was treated
> differently than OMP_* and OACC_HOST_DATA; your order is a leftover
> from that code movement/change.)
I've fixed this bit -- which actually doesn't need the goto any more
either, so that's now a fallthrough instead.
> > + flags = GOVD_MAP | GOVD_EXPLICIT;
> > + if (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ALWAYS_TO
> > +
Re: Fortran: Use non conflicting file extensions for intermediates [PR81615]
Am 20.12.23 um 05:32 schrieb Rimvydas Jasinskas: Dear all, In the spirit of c/c++ using the .i/.ii extensions for intermediates, use the .fi/.fii intermediate extensions for gfortran fixed/free form sources when -save-temps is invoked to avoid various issues. I checked with Jerry on Mattermost that there are no objections regarding the suffixes. Pushed: https://gcc.gnu.org/g:ba615557a4c698d27042a5fe058ea6e721a03b12 Thanks for the patch! The documentation part will be submitted separately, because it involves adding a "Developer options" mini-section (as suggested by Harald) and moving the -fdump-* options from "Options for debugging your program or GNU Fortran" section. Documentation improvements are always great! Regards, Rimvydas Cheers, Harald
[PATCH] [OpenACC] Add tests for implied copy of variables in reduction clause.
From: Hafiz Abid Qadeer
The OpenACC reduction clause on compute construct implies a copy clause
for each reduction variable [1]. This patch adds tests to check if the
implied copy is being generated. The check covers various types and
operators as described in the specification.
gcc/testsuite/ChangeLog:
* c-c++-common/goacc/implied-copy-1.c: New test.
* c-c++-common/goacc/implied-copy-2.c: New test.
* g++.dg/goacc/implied-copy.C: New test.
* gcc.dg/goacc/implied-copy.c: New test.
* gfortran.dg/goacc/implied-copy-1.f90: New test.
* gfortran.dg/goacc/implied-copy-2.f90: New test.
[1] OpenACC 2.7 Specification section 2.5.13
---
.../c-c++-common/goacc/implied-copy-1.c | 33
.../c-c++-common/goacc/implied-copy-2.c | 121 +
gcc/testsuite/g++.dg/goacc/implied-copy.C | 24 +++
gcc/testsuite/gcc.dg/goacc/implied-copy.c | 29
.../gfortran.dg/goacc/implied-copy-1.f90 | 35
.../gfortran.dg/goacc/implied-copy-2.f90 | 160 ++
6 files changed, 402 insertions(+)
create mode 100644 gcc/testsuite/c-c++-common/goacc/implied-copy-1.c
create mode 100644 gcc/testsuite/c-c++-common/goacc/implied-copy-2.c
create mode 100644 gcc/testsuite/g++.dg/goacc/implied-copy.C
create mode 100644 gcc/testsuite/gcc.dg/goacc/implied-copy.c
create mode 100644 gcc/testsuite/gfortran.dg/goacc/implied-copy-1.f90
create mode 100644 gcc/testsuite/gfortran.dg/goacc/implied-copy-2.f90
diff --git a/gcc/testsuite/c-c++-common/goacc/implied-copy-1.c
b/gcc/testsuite/c-c++-common/goacc/implied-copy-1.c
new file mode 100644
index 000..ae06339dc2d
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/goacc/implied-copy-1.c
@@ -0,0 +1,33 @@
+/* { dg-additional-options "-fdump-tree-gimple" } */
+
+/* Test for implied copy of reduction variable on combined construct. */
+void test1 (void)
+{
+ int i, sum = 0, prod = 1, a[100];
+
+ #pragma acc kernels loop reduction(+:sum) reduction(*:prod)
+ for (int i = 0; i < 10; ++i)
+ {
+sum += a[i];
+prod *= a[i];
+ }
+
+ #pragma acc parallel loop reduction(+:sum) reduction(*:prod)
+ for (int i = 0; i < 10; ++i)
+ {
+sum += a[i];
+prod *= a[i];
+ }
+
+ #pragma acc serial loop reduction(+:sum) reduction(*:prod)
+ for (int i = 0; i < 10; ++i)
+ {
+sum += a[i];
+prod *= a[i];
+ }
+}
+
+/* { dg-final { scan-tree-dump-times "map\\(force_tofrom:sum \\\[len:
\[0-9\]+\\\]\\)" 1 "gimple" } } */
+/* { dg-final { scan-tree-dump-times "map\\(force_tofrom:prod \\\[len:
\[0-9\]+\\\]\\)" 1 "gimple" } } */
+/* { dg-final { scan-tree-dump-times "map\\(tofrom:sum \\\[len:
\[0-9\]+\\\]\\)" 2 "gimple" } } */
+/* { dg-final { scan-tree-dump-times "map\\(tofrom:prod \\\[len:
\[0-9\]+\\\]\\)" 2 "gimple" } } */
diff --git a/gcc/testsuite/c-c++-common/goacc/implied-copy-2.c
b/gcc/testsuite/c-c++-common/goacc/implied-copy-2.c
new file mode 100644
index 000..124f128964d
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/goacc/implied-copy-2.c
@@ -0,0 +1,121 @@
+/* { dg-additional-options "-fdump-tree-gimple" } */
+
+/* Test that reduction on compute construct implies a copy of the reduction
+ variable . */
+
+#define n 1000
+
+#if __cplusplus
+ typedef bool BOOL;
+#else
+ typedef _Bool BOOL;
+#endif
+
+int
+main(void)
+{
+ int i;
+ int sum = 0;
+ int prod = 1;
+ int result = 0;
+ int tmp = 1;
+ int array[n];
+
+ double sumd = 0.0;
+ double arrayd[n];
+
+ float sumf = 0.0;
+ float arrayf[n];
+
+ char sumc;
+ char arrayc[n];
+
+ BOOL lres;
+
+#pragma acc parallel reduction(+:sum, sumf, sumd, sumc) reduction(*:prod)
+ for (i = 0; i < n; i++)
+{
+ sum += array[i];
+ sumf += arrayf[i];
+ sumd += arrayd[i];
+ sumc += arrayc[i];
+ prod *= array[i];
+}
+
+#pragma acc parallel reduction (max:result)
+ for (i = 0; i < n; i++)
+result = result > array[i] ? result : array[i];
+
+#pragma acc parallel reduction (min:result)
+ for (i = 0; i < n; i++)
+result = result < array[i] ? result : array[i];
+
+#pragma acc parallel reduction (&:result)
+ for (i = 0; i < n; i++)
+result &= array[i];
+
+#pragma acc parallel reduction (|:result)
+ for (i = 0; i < n; i++)
+result |= array[i];
+
+#pragma acc parallel reduction (^:result)
+ for (i = 0; i < n; i++)
+result ^= array[i];
+
+#pragma acc parallel reduction (&&:lres) copy(tmp)
+ for (i = 0; i < n; i++)
+lres = lres && (tmp > array[i]);
+
+#pragma acc parallel reduction (||:lres) copy(tmp)
+ for (i = 0; i < n; i++)
+lres = lres || (tmp > array[i]);
+
+ /* Same checks on serial construct. */
+#pragma acc serial reduction(+:sum, sumf, sumd, sumc) reduction(*:prod)
+ for (i = 0; i < n; i++)
+{
+ sum += array[i];
+ sumf += arrayf[i];
+ sumd += arrayd[i];
+ sumc += arrayc[i];
+ prod *= array[i];
+}
+
+#pragma acc serial reduction (max:result)
+ for (i = 0; i < n; i++)
+result = result > array[i] ? result : array[i];
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
