Re: [PATCH] openmp, fortran: Check that event handles passed to detach clauses are not arrays [PR104131]

[Jakub Jelinek via Fortran]

On Tue, Mar 01, 2022 at 08:58:54AM +0100, Tobias Burnus wrote:
> The wording actually also permits array sections. But contrary to coarrays,
> (which are odd but otherwise fine), I think it does not really make sense
> to have arrays and array sections here.
> 
> (Do we need/want to get this clarified/changed in spec?)

In 5.2 we have:
"A variable that is part of another variable (as an array element or a 
structure element) cannot
appear in a detach clause."
and
"An array section can appear only in clauses for which it is explicitly
allowed."
and
C/C++
"A variable of  OpenMP type is a variable of type 
omp__t."
and
Fortran
"A variable of  OpenMP type is a scalar integer variable of kind
omp__kind."
and
"event-handle   variable of event_handle type   default"

As there is no explicit allowing of array sections, array sections aren't
allowed in detach, and it must be scalar integer.
The question is if a non-coindexed coarray is a scalar integer variable or
not.

Jakub

Re: [PATCH] openmp, fortran: Check that event handles passed to detach clauses are not arrays [PR104131]

[Tobias Burnus]

First, thanks for looking into the standard. I think the information is
too much spread through the standard. I keep searching for restrictions,
find them at 5 places and miss another 5. With OpenMP 5.2, it became
even worse.

On 01.03.22 09:16, Jakub Jelinek wrote:

As there is no explicit allowing of array sections, array sections aren't
allowed in detach, and it must be scalar integer.
The question is if a non-coindexed coarray is a scalar integer variable or
not.


I think the Fortran sense, yes. That only coindexed vars are disallowed
also implies this in the OpenMP spec. In terms of the implementation,
a local part of a coarray is really not much different from any other
variable, except that all coarrays have to be either in static memory
or allocatables/pointers (which are collectively allocated).

For a non-descriptor variable like:

program main
  integer, save :: a[*]
  call foo(a)
contains
  subroutine foo(x)
   integer :: x[*]
  end
end

That's also the case for the internal representation:

  static void * restrict caf_token.2;
  static integer(kind=4) * restrict a;

  void foo (integer(kind=4) * restrict x,
void * restrict caf_token.0,
integer(kind=8) caf_offset.1)
'x' is a pretty normal variable (admittedly, internally now a pointer)
which can be used like a noncoarray. (The pointer is there to permit
to put the var into some special, remote accessible memory.)

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] openmp, fortran: Check that event handles passed to detach clauses are not arrays [PR104131]

[Mikael Morin]

So, if I try to sum up what has been gathered in this thread:

 - pr104131.f90 is invalid, as x is not scalar.
   Checks are better done in resolve_omp_clauses after a call
   to gfc_resolve_expr.
   Checking expr->sym->attr.dimension seems to cover more cases than
   expr->rank > 0.

 - pr104131-2.f90 is valid and should be accepted.

 - Some other cases should be rejected, including x[1] (coindexed
   variable), x(1) (array element), x%comp (structure component).

Is that correct? Anything else?

Regarding the expr->rank vs expr->sym->attr.dimension controversy, my 
take is that it should stick to the error message.  Use expr->rank is 
the error is about scalar vs array, use expr->sym->attr.dimension if 
it’s about subobject-ness of an array variable.


Coming back to the PR, the ICE backtraces for pr104131.f90 and 
pr104131-2.f90 are different and should probably be treated separatedly.
I don’t know how difficult the bullet 2 above would be, but bullet 1 and 
3 seem quite doable.

Re: OpenACC 'kernels' decomposition: Mark variables used in synthesized data clauses as addressable [PR100280]

[Thomas Schwinge]

Hi!

Jakub, need your review/approval here, please:

On 2022-01-13T10:54:16+0100, I wrote:
> On 2019-05-08T14:51:57+0100, Julian Brown  wrote:
>>  - The "addressable" bit is set during the kernels conversion pass for
>>variables that have "create" (alloc) clauses created for them in the
>>synthesised outer data region (instead of in the front-end, etc.,
>>where it can't be done accurately). Such variables actually have
>>their address taken during transformations made in a later pass
>>(omp-low, I think), but there's a phase-ordering problem that means
>>the flag should be set earlier.
>
> The actual issue is a bit different, but yes, there is a problem.
> The related ICE has also been reported as 
> "ICE in lower_omp_target, at omp-low.c:12287".  (And I'm confused why we
> didn't run into that with the OpenACC 'kernels' decomposition
> originally.)  I've pushed to master branch
> commit 9b32c1669aad5459dd053424f9967011348add83
> "OpenACC 'kernels' decomposition: Mark variables used in synthesized data
> clauses as addressable [PR100280]"

> ... as otherwise 'gcc/omp-low.c:lower_omp_target' has to create a temporary:
>
> 13073 else if (is_gimple_reg (var))
> 13074   {
> 13075 gcc_assert (offloaded);
> 13076 tree avar = create_tmp_var (TREE_TYPE 
> (var));
> 13077 mark_addressable (avar);
>
> ..., which (a) is only implemented for actualy *offloaded* regions (but not
> data regions), and (b) the subsequently synthesized code for writing to and
> later reading back from the temporary fundamentally conflicts with OpenACC
> 'async' (as used by OpenACC 'kernels' decomposition).  That's all not trivial
> to make work, so let's just avoid this case.

> --- a/gcc/omp-oacc-kernels-decompose.cc
> +++ b/gcc/omp-oacc-kernels-decompose.cc
> @@ -793,7 +793,8 @@ make_data_region_try_statement (location_t loc, gimple 
> *body)
>
>  /* If INNER_BIND_VARS holds variables, build an OpenACC data region with
> location LOC containing BODY and having 'create (var)' clauses for each
> -   variable.  If INNER_CLEANUP is present, add a try-finally statement with
> +   variable (as a side effect, such variables also get TREE_ADDRESSABLE set).
> +   If INNER_CLEANUP is present, add a try-finally statement with
> this cleanup code in the finally block.  Return the new data region, or
> the original BODY if no data region was needed.  */
>
> @@ -842,6 +843,9 @@ maybe_build_inner_data_region (location_t loc, gimple 
> *body,
> inner_data_clauses = new_clause;
>
> prev_mapped_var = v;
> +
> +   /* See .  */
> +   TREE_ADDRESSABLE (v) = 1;
>   }
>  }

So, that's too simple.  ;-) ... and gives rise to workaround patches like
we have on the og11 development branch:
  - "Avoid introducing 'create' mapping clauses for loop index variables in 
kernels regions",
  - "Run all kernels regions with GOMP_MAP_FORCE_TOFROM mappings synchronously",
  - "Fix for is_gimple_reg vars to 'data kernels'"

We're after gimplification, and must not just set 'TREE_ADDRESSABLE',
because that may easily violate GIMPLE invariants, leading to ICEs later.
There are a few open PRs, which my following changes are addressing.  To
make "late" 'TREE_ADDRESSABLE' work, we have a precedent in OpenMP's
'gcc/omp-low.cc:task_shared_vars' handling, as Jakub had pointed to in
discussion of .  (PR102330 turned out to be
unrelated from the "late" 'TREE_ADDRESSABLE' problem here; I have a
different patch for it.)

I'm thus proposing to generalize 'gcc/omp-low.cc:task_shared_vars' into
'make_addressable_vars', plus new 'OMP_CLAUSE_MAP_DECL_MAKE_ADDRESSABLE'
that we then may use instead of the 'TREE_ADDRESSABLE (v) = 1;' quoted
above (plus one or two additional ones to be introduced in later
patches), and wire that up in 'gcc/omp-low.cc:scan_sharing_clauses', for
'OMP_CLAUSE_MAP': set 'TREE_ADDRESSABLE' and put into
'make_addressable_vars' for later fix-up.

(In reply to Jakub Jelinek from comment #9)
> Whether you can use the same bitmap or need to add another bitmap next to
> task_shared_vars is something hard to guess without diving into it deeply.

Per my understanding of the code, the only place where I had doubts is
'gcc/omp-low.cc:finish_taskreg_scan', but I have convinced myself that
what this is doing is either a no-op in the
'OMP_CLAUSE_MAP_DECL_MAKE_ADDRESSABLE' case, or in fact necessary as the
original 'task_shared_vars' handling has been.  Either way: I couldn't
come up with a way (test case) that we'd actually run into this case;
you'd have to have the relevant OpenMP constructs inside an OpenACC
'kernels' region, which isn't permitted per
'gcc/omp-low.cc:check_omp_nesting_restrictions'.

OK to proceed in this way?


Grüße
 Thomas


--- gcc/omp-low.cc
+++ gcc/omp-low.cc
@@ -188,7 +1

[PATCH, v2] PR fortran/104573 - ICE in resolve_structure_cons, at fortran/resolve.cc:1299

[Harald Anlauf via Fortran]

Hi Mikael,

Am 28.02.22 um 22:38 schrieb Mikael Morin:

Le 28/02/2022 à 22:32, Mikael Morin a écrit :

So please use a condition on expr->ts.type instead.
I said «instead», but «as well» is more appropriate; both expr.ts.type

and expr.ts.u.derived conditions are probably necessary.



I do hope I got you right.  The attached patch fixes your variant
as well as the original testcase, and regtests fine.
Just to be sure: is this what you were thinking of?

Thanks for the very constructive review!

Harald
From e4816e150c31e127c3b6dc0032ae533a2d42 Mon Sep 17 00:00:00 2001
From: Harald Anlauf 
Date: Tue, 1 Mar 2022 23:13:17 +0100
Subject: [PATCH] Fortran: error recovery after invalid assumed type
 declaration

gcc/fortran/ChangeLog:

	PR fortran/104573
	* resolve.cc (resolve_structure_cons): Avoid NULL pointer
	dereference when there is no valid component.

gcc/testsuite/ChangeLog:

	PR fortran/104573
	* gfortran.dg/assumed_type_14.f90: New test.
---
 gcc/fortran/resolve.cc| 10 ++---
 gcc/testsuite/gfortran.dg/assumed_type_14.f90 | 22 +++
 2 files changed, 29 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/gfortran.dg/assumed_type_14.f90

diff --git a/gcc/fortran/resolve.cc b/gcc/fortran/resolve.cc
index 753aa27e23f..0afa5d3346a 100644
--- a/gcc/fortran/resolve.cc
+++ b/gcc/fortran/resolve.cc
@@ -1288,15 +1288,19 @@ resolve_structure_cons (gfc_expr *expr, int init)
 	}
 }
 
-  cons = gfc_constructor_first (expr->value.constructor);
-
   /* A constructor may have references if it is the result of substituting a
  parameter variable.  In this case we just pull out the component we
  want.  */
   if (expr->ref)
 comp = expr->ref->u.c.sym->components;
-  else
+  else if ((expr->ts.type == BT_DERIVED || expr->ts.type == BT_CLASS
+	|| expr->ts.type == BT_UNION)
+	   && expr->ts.u.derived)
 comp = expr->ts.u.derived->components;
+  else
+return false;
+
+  cons = gfc_constructor_first (expr->value.constructor);
 
   for (; comp && cons; comp = comp->next, cons = gfc_constructor_next (cons))
 {
diff --git a/gcc/testsuite/gfortran.dg/assumed_type_14.f90 b/gcc/testsuite/gfortran.dg/assumed_type_14.f90
new file mode 100644
index 000..112cde34b27
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/assumed_type_14.f90
@@ -0,0 +1,22 @@
+! { dg-do compile }
+! PR fortran/104573 - ICE in resolve_structure_cons
+! Contributed by G.Steinmetz
+! Contributed by M.Morin
+
+program p
+  type t
+  end type
+  type(*), parameter :: x = t() ! { dg-error "Assumed type of variable" }
+  print *, x
+end
+
+subroutine s
+  type t
+ integer :: a
+  end type
+  character(3), parameter :: x = t(2) ! { dg-error "Cannot convert" }
+  character(3), parameter :: y = x! { dg-error "Unclassifiable statement" }
+  print *, y
+end
+
+! { dg-prune-output "Cannot convert" }
-- 
2.34.1