[PATCH, v2, OpenMP 5.2, Fortran] Strictly-structured block support for OpenMP directives
Hi Jakub,
this version adjusts the patch to let sections/parallel sections also use
strictly-structured blocks, making it more towards 5.2.
Because of this change, some of the testcases using the sections-construct need
a bit of adjustment too, since "block; end block" at the start of the construct
now means something different than before.
There are now three new testcases, with the non-dg-error/dg-error cases
separated,
and a third testcase containing a few cases listed in prior emails. I hope this
is
enough.
The implementation status entry in libgomp/libgomp.texi for strictly-structured
blocks
has also been changed to "Y" in this patch.
Tested without regressions, is this now okay for trunk?
Thanks,
Chung-Lin
2021-10-20 Chung-Lin Tang
gcc/fortran/ChangeLog:
* decl.c (gfc_match_end): Add COMP_OMP_STRICTLY_STRUCTURED_BLOCK case
together with COMP_BLOCK.
* parse.c (parse_omp_structured_block): Change return type to
'gfc_statement', add handling for strictly-structured block case, adjust
recursive calls to parse_omp_structured_block.
(parse_executable): Adjust calls to parse_omp_structured_block.
* parse.h (enum gfc_compile_state): Add
COMP_OMP_STRICTLY_STRUCTURED_BLOCK.
* trans-openmp.c (gfc_trans_omp_workshare): Add EXEC_BLOCK case
handling.
gcc/testsuite/ChangeLog:
* gfortran.dg/gomp/cancel-1.f90: Adjust testcase.
* gfortran.dg/gomp/nesting-3.f90: Adjust testcase.
* gfortran.dg/gomp/strictly-structured-block-1.f90: New test.
* gfortran.dg/gomp/strictly-structured-block-2.f90: New test.
* gfortran.dg/gomp/strictly-structured-block-3.f90: New test.
libgomp/ChangeLog:
* libgomp.texi (Support of strictly structured blocks in Fortran):
Adjust to 'Y'.
* testsuite/libgomp.fortran/task-reduction-16.f90: Adjust testcase.
diff --git a/gcc/fortran/decl.c b/gcc/fortran/decl.c
index d6a22d13451..66489da12be 100644
--- a/gcc/fortran/decl.c
+++ b/gcc/fortran/decl.c
@@ -8449,6 +8449,7 @@ gfc_match_end (gfc_statement *st)
break;
case COMP_BLOCK:
+case COMP_OMP_STRICTLY_STRUCTURED_BLOCK:
*st = ST_END_BLOCK;
target = " block";
eos_ok = 0;
diff --git a/gcc/fortran/parse.c b/gcc/fortran/parse.c
index 7d765a0866d..2fb98844356 100644
--- a/gcc/fortran/parse.c
+++ b/gcc/fortran/parse.c
@@ -5451,7 +5451,7 @@ parse_oacc_loop (gfc_statement acc_st)
/* Parse the statements of an OpenMP structured block. */
-static void
+static gfc_statement
parse_omp_structured_block (gfc_statement omp_st, bool workshare_stmts_only)
{
gfc_statement st, omp_end_st;
@@ -5538,6 +5538,32 @@ parse_omp_structured_block (gfc_statement omp_st, bool
workshare_stmts_only)
gcc_unreachable ();
}
+ bool block_construct = false;
+ gfc_namespace *my_ns = NULL;
+ gfc_namespace *my_parent = NULL;
+
+ st = next_statement ();
+
+ if (st == ST_BLOCK)
+{
+ /* Adjust state to a strictly-structured block, now that we found that
+the body starts with a BLOCK construct. */
+ s.state = COMP_OMP_STRICTLY_STRUCTURED_BLOCK;
+
+ block_construct = true;
+ gfc_notify_std (GFC_STD_F2008, "BLOCK construct at %C");
+
+ my_ns = gfc_build_block_ns (gfc_current_ns);
+ gfc_current_ns = my_ns;
+ my_parent = my_ns->parent;
+
+ new_st.op = EXEC_BLOCK;
+ new_st.ext.block.ns = my_ns;
+ new_st.ext.block.assoc = NULL;
+ accept_statement (ST_BLOCK);
+ st = parse_spec (ST_NONE);
+}
+
do
{
if (workshare_stmts_only)
@@ -5554,7 +5580,6 @@ parse_omp_structured_block (gfc_statement omp_st, bool
workshare_stmts_only)
restrictions apply recursively. */
bool cycle = true;
- st = next_statement ();
for (;;)
{
switch (st)
@@ -5580,13 +5605,13 @@ parse_omp_structured_block (gfc_statement omp_st, bool
workshare_stmts_only)
case ST_OMP_PARALLEL_MASKED:
case ST_OMP_PARALLEL_MASTER:
case ST_OMP_PARALLEL_SECTIONS:
- parse_omp_structured_block (st, false);
- break;
+ st = parse_omp_structured_block (st, false);
+ continue;
case ST_OMP_PARALLEL_WORKSHARE:
case ST_OMP_CRITICAL:
- parse_omp_structured_block (st, true);
- break;
+ st = parse_omp_structured_block (st, true);
+ continue;
case ST_OMP_PARALLEL_DO:
case ST_OMP_PARALLEL_DO_SIMD:
@@ -5609,7 +5634,7 @@ parse_omp_structured_block (gfc_statement omp_st, bool
workshare_stmts_only)
}
}
else
- st = parse_executable (ST_NONE);
+ st = parse_executable (st);
if (st == ST_NONE)
unexpected_eof ();
else if (st == ST_OMP_SECTION
@@ -5619,9 +5644,27 @@ parse_omp_structured_block (
Re: [PATCH, v2, OpenMP 5.2, Fortran] Strictly-structured block support for OpenMP directives
On Wed, Oct 20, 2021 at 08:30:34PM +0800, Chung-Lin Tang wrote:
> 2021-10-20 Chung-Lin Tang
>
> gcc/fortran/ChangeLog:
>
> * decl.c (gfc_match_end): Add COMP_OMP_STRICTLY_STRUCTURED_BLOCK case
> together with COMP_BLOCK.
> * parse.c (parse_omp_structured_block): Change return type to
> 'gfc_statement', add handling for strictly-structured block case, adjust
> recursive calls to parse_omp_structured_block.
> (parse_executable): Adjust calls to parse_omp_structured_block.
> * parse.h (enum gfc_compile_state): Add
> COMP_OMP_STRICTLY_STRUCTURED_BLOCK.
> * trans-openmp.c (gfc_trans_omp_workshare): Add EXEC_BLOCK case
> handling.
>
> gcc/testsuite/ChangeLog:
>
> * gfortran.dg/gomp/cancel-1.f90: Adjust testcase.
> * gfortran.dg/gomp/nesting-3.f90: Adjust testcase.
> * gfortran.dg/gomp/strictly-structured-block-1.f90: New test.
> * gfortran.dg/gomp/strictly-structured-block-2.f90: New test.
> * gfortran.dg/gomp/strictly-structured-block-3.f90: New test.
>
> libgomp/ChangeLog:
>
> * libgomp.texi (Support of strictly structured blocks in Fortran):
> Adjust to 'Y'.
> * testsuite/libgomp.fortran/task-reduction-16.f90: Adjust testcase.
Thanks, looks mostly good now, but I still have nits for the testsuite.
> --- /dev/null
> +++ b/gcc/testsuite/gfortran.dg/gomp/strictly-structured-block-1.f90
> @@ -0,0 +1,211 @@
> +! { dg-do compile }
> +! { dg-options "-fopenmp" }
> +
> +program main
> + integer :: x, i, n
> +
> + !$omp parallel
> + block
> +x = x + 1
> + end block
I'd prefer not to use those x = j or x = x + 1 etc.
as statements that do random work here whenever possible.
While those are dg-do compile testcases, especially if
it is without dg-errors I think it is preferrable not to show
bad coding examples.
E.g. the x = x + 1 above is wrong for 2 reasons, x is uninitialized
before the parallel, and there is a data race, the threads, teams etc.
can write to x concurrently.
I think better would be to use something like
call do_work
which doesn't have to be defined anywhere and will just stand there
as a black box for unspecified work.
> + !$omp workshare
> + block
> +x = x + 1
> + end block
There are exceptions though, e.g. workshare is such a case, because
e.g. call do_work is not valid in workshare.
So, it is ok to keep using x = x + 1 here if you initialize it
first at the start of the program.
> + !$omp workshare
> + block
> +x = 1
> +!$omp critical
> +block
> + x = 3
> +end block
> + end block
And then there are cases like the above, please
just use different variables there (all initialized) or
say an array and access different elements in the different spots.
Jakub
Re: [PATCH][WIP] Add install-dvi Makefile targets
On Tue, Oct 19, 2021 at 1:41 AM Thomas Koenig wrote: > > Hi Eric, > > > Hi, I have updated this patch and tested it with more languages now; I > > can now confirm that it works with ada, d, and fortran now. The only > > languages that remain untested now are go (since I'm building on > > darwin and go doesn't build on darwin anyways, as per bug 46986) and > > jit (which I ran into a bug about that I brought up on IRC, and will > > probably need to file on bugzilla). OK to install? > > Fortran parts look good. > > Best regards > > Thomas OK, thanks... so... at this point, who do I still need approval from for the rest of it, then? Should I be cc-ing the build system maintainers? The maintainers for all the rest of the subdirectories I'm touching? Global reviewers? Someone else? Thanks, Eric Gallager
[PATCH] Fortran: Fixes and additional tests for shape/ubound/size [PR94070]
This patch started out as some additional testcases for the SHAPE,
UBOUND, and SIZE intrinsic extensions for assumed-rank arrays added by
TS29113; I realized a while ago that I had not added test coverage for
polymorphic arguments. My guess that this was a likely trouble spot was
correct as the new test cases did not work. :-(
The one that was most concerning was an ICE when calling the SHAPE
intrinsic with an assumed-rank class type argument, as reported in
PR94070. (I think this ICE is similar to the one reported in PR102641
that Tobias thinks is a problem with the scalarizer.) In this case,
SHAPE was calling a library function and trying to copy the array
contents to a temporary, which is really stupid because SHAPE only needs
to look at the descriptor and not the array contents. I thought we
could handle this inline the same as UBOUND and LBOUND, by extending
gfc_trans_intrinsic_bound, and avoid the library function entirely.
Then, I found some other existing problems in gfc_trans_intrinsic_bound;
the conditional it was building to test for the extent-zero special
cases for LBOUND and UBOUND was completely wrong, and the compile-time
test for the assumed-rank/assumed-size case was wrong too. So I ended
up rewriting large parts of that function.
I also fixed a bug in the SIZE intrinsic where it was not taking the
class types into account. (SIZE is already being handled inline in a
separate place, otherwise I might've merged it into
gfc_trans_intrinsic_bound as well.)
While I was at it I also added 3 more testcases for these functions to
test for correct behavior with bind(c). All 6 new tests PASS now, and
there are no other regressions.
OK to commit?
-Sandra
commit c74d3f5ae059b74a552428d6f1602885ca239094
Author: Sandra Loosemore
Date: Tue Oct 19 21:11:15 2021 -0700
Fortran: Fixes and additional tests for shape/ubound/size [PR94070]
This patch reimplements the SHAPE intrinsic to be inlined similarly to
LBOUND and UBOUND, instead of as a library call, to avoid an
unnecessary array copy. Various bugs are also fixed.
gcc/fortran/
PR fortran/94070
* expr.c (gfc_simplify_expr): Handle GFC_ISYM_SHAPE along with
GFC_ISYM_LBOUND and GFC_ISYM_UBOUND.
* trans-array.c (gfc_conv_ss_startstride): Likewise.
(set_loop_bounds): Likewise.
(gfc_tree_array_size): Handle class arrays.
* trans-intrinsic.c (gfc_trans_intrinsic_bound): Extend to
handle SHAPE. Correct logic for zero-size special cases and
detecting assumed-rank arrays associated with an assumed-size
argument.
(gfc_conv_intrinsic_shape): Deleted.
(gfc_conv_intrinsic_function): Handle GFC_ISYM_SHAPE like
GFC_ISYM_LBOUND and GFC_ISYM_UBOUND.
(gfc_add_intrinsic_ss_code): Likewise.
(gfc_walk_intrinsic_bound): Likewise.
gcc/testsuite/gfortran.dg/
PR fortran/94070
* c-interop/shape-bindc.f90: New test.
* c-interop/shape-poly.f90: New test.
* c-interop/size-bindc.f90: New test.
* c-interop/size-poly.f90: New test.
* c-interop/ubound-bindc.f90: New test.
* c-interop/ubound-poly.f90: New test.
diff --git a/gcc/fortran/expr.c b/gcc/fortran/expr.c
index 66f24c6..b19d3a2 100644
--- a/gcc/fortran/expr.c
+++ b/gcc/fortran/expr.c
@@ -2205,7 +2205,8 @@ gfc_simplify_expr (gfc_expr *p, int type)
(p->value.function.isym->id == GFC_ISYM_LBOUND
|| p->value.function.isym->id == GFC_ISYM_UBOUND
|| p->value.function.isym->id == GFC_ISYM_LCOBOUND
- || p->value.function.isym->id == GFC_ISYM_UCOBOUND))
+ || p->value.function.isym->id == GFC_ISYM_UCOBOUND
+ || p->value.function.isym->id == GFC_ISYM_SHAPE))
ap = ap->next;
for ( ; ap; ap = ap->next)
diff --git a/gcc/fortran/trans-array.c b/gcc/fortran/trans-array.c
index f8c087e..323edcb 100644
--- a/gcc/fortran/trans-array.c
+++ b/gcc/fortran/trans-array.c
@@ -4507,6 +4507,7 @@ gfc_conv_ss_startstride (gfc_loopinfo * loop)
case GFC_ISYM_UBOUND:
case GFC_ISYM_LCOBOUND:
case GFC_ISYM_UCOBOUND:
+ case GFC_ISYM_SHAPE:
case GFC_ISYM_THIS_IMAGE:
loop->dimen = ss->dimen;
goto done;
@@ -4558,12 +4559,14 @@ done:
/* Fall through to supply start and stride. */
case GFC_ISYM_LBOUND:
case GFC_ISYM_UBOUND:
+ /* This is the variant without DIM=... */
+ gcc_assert (expr->value.function.actual->next->expr == NULL);
+ /* Fall through. */
+
+ case GFC_ISYM_SHAPE:
{
gfc_expr *arg;
- /* This is the variant without DIM=... */
- gcc_assert (expr->value.function.actual->next->expr == NULL);
-
arg = expr->value.function.actual->expr;
if (arg->rank == -1)
{
@@ -5350,10 +5353,13 @@ set_loop_bounds (gfc_loopinfo *loop)
gfc_expr *expr = loopspec[n]->info->expr;
/* The {l,u}bound of an assumed rank. */
- gcc_assert ((expr->value.function.isym->id == GFC_ISYM_LBOUND
- || expr->value.function.isym->id == G
Re: [PATCH] Fortran: Fixes and additional tests for shape/ubound/size [PR94070]
Hi Sandra,
On 20.10.21 22:03, Sandra Loosemore wrote:
The one that was most concerning was an ICE when calling the SHAPE
intrinsic with an assumed-rank class type argument ... In this case,
SHAPE was calling a library function and trying to copy the array
contents to a temporary, which is really stupid because SHAPE only
needs to look at the descriptor and not the array contents. I thought
we could handle this inline the same as UBOUND and LBOUND, by
extending gfc_trans_intrinsic_bound, and avoid the library function
entirely.
Then, I found some other existing problems in
gfc_trans_intrinsic_bound; the conditional it was building to test for
the extent-zero special cases for LBOUND and UBOUND was completely
wrong, and the compile-time test for the assumed-rank/assumed-size
case was wrong too. So I ended up rewriting large parts of that
function.
I also fixed a bug in the SIZE intrinsic where it was not taking the
class types into account. (SIZE is already being handled inline in a
separate place, otherwise I might've merged it into
gfc_trans_intrinsic_bound as well.)
Thanks for your efforts!
LGTM with the changelog path fix, without the gfc_tree_array_size
attribute change and with the indentation fix.
Namely:
gcc/testsuite/gfortran.dg/
PR fortran/94070
* c-interop/shape-bindc.f90: New test.
The ChangeLog file is in testsuite/ not in testsuite/gfortran.dg/.
@@ -8054,9 +8060,18 @@ gfc_tree_array_size (stmtblock_t *block, tree desc,
gfc_expr *expr, tree dim)
return GFC_TYPE_ARRAY_SIZE (TREE_TYPE (desc));
}
tree size, tmp, rank = NULL_TREE, cond = NULL_TREE;
- symbol_attribute attr = gfc_expr_attr (expr);
+ symbol_attribute attr;
gfc_array_spec *as = gfc_get_full_arrayspec_from_expr (expr);
gcc_assert (GFC_DESCRIPTOR_TYPE_P (TREE_TYPE (desc)));
+
+ if (expr->ts.type == BT_CLASS)
+{
+ attr = CLASS_DATA (expr->symtree->n.sym)->attr;
+ attr.pointer = attr.class_pointer;
+}
+ else
+attr = gfc_expr_attr (expr);
Short version: Can you undo this change and verify that it still
fails? – Because I think that it works now due to the patch mentioned below.
At least it did pass here and an assert only pointed to testcases,
where I expected an issue.
* * *
Long version:
I stumbled over this while reading the patch as I think it is wrong in the
general case. However, I belief it is fine for the particular use
(only allocatable + pointer with assumed-rank arrays). It does mishandle
"nonclass%class_comp" – but that is inaccessible if 'nonclass' is
assumed-rank (and the attributes are only used in that case).
Nonetheless, I fail to see when this fails - because I think that gfc_expr_attr
should yield the proper result (since Tue Oct 12, my
eb92cd57a1ebe7cd7589bdbec34d9ae337752ead)
I think before before that patch, the problem was that expr was not 'var'
but 'var%_data' and gfc_expr then returned the attributes for gfc_expr,
which always had attr.pointer == 1 as the true 'pointer' attribute is in
attr.class_pointer and not in attr.pointer.
My bet is that you modified this before doing the "git pull" which pulled
in my patch above ...
For testing, I did turn your change into an assert and it only failed for
class_48.f90 and pr93792.f90. Those expose the issue I was concerned about:
(gdb) p gfc_debug_expr(expr)
test4:one % a % _data(FULL)
(gdb) p gfc_debug_expr(expr)
copy:self % x % _data(FULL)
(As said: nonissue in this case, but still feels wrong – and as the
workaround is longer be needed, it can also be removed.)
--- a/gcc/fortran/trans-intrinsic.c
+++ b/gcc/fortran/trans-intrinsic.c
...
+ /* Descriptors for assumed-size arrays have ubound = -1
+ in the last dimension. */
+ cond1 = fold_build2_loc (input_location, EQ_EXPR,
+ logical_type_node, ubound, minus_one);
Here, indentation goes wrong.
Thanks,
Tobias
-
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] Fortran: Fixes and additional tests for shape/ubound/size [PR94070]
On 10/20/21 3:41 PM, Tobias Burnus wrote: Hi Sandra, On 20.10.21 22:03, Sandra Loosemore wrote: The one that was most concerning was an ICE when calling the SHAPE intrinsic with an assumed-rank class type argument ... In this case, SHAPE was calling a library function and trying to copy the array contents to a temporary, which is really stupid because SHAPE only needs to look at the descriptor and not the array contents. I thought we could handle this inline the same as UBOUND and LBOUND, by extending gfc_trans_intrinsic_bound, and avoid the library function entirely. Then, I found some other existing problems in gfc_trans_intrinsic_bound; the conditional it was building to test for the extent-zero special cases for LBOUND and UBOUND was completely wrong, and the compile-time test for the assumed-rank/assumed-size case was wrong too. So I ended up rewriting large parts of that function. I also fixed a bug in the SIZE intrinsic where it was not taking the class types into account. (SIZE is already being handled inline in a separate place, otherwise I might've merged it into gfc_trans_intrinsic_bound as well.) Thanks for your efforts! LGTM with the changelog path fix, without the gfc_tree_array_size attribute change and with the indentation fix. Thanks, committed now with those fixes. You are right that I'd made that change to gfc_tree_array_size before I pulled your alternate fix for that, and didn't realize that my hack had become redundant. -Sandra
