[OpenMP,5.1,Fortran] Strictly-structured block support for OpenMP directives

  Hi all,
this patch add support for "strictly-structured blocks" introduced in OpenMP 5.1,
basically allowing BLOCK constructs to serve as the body for directives:

!$omp target
block
   ...
end block
[!$omp end target]  !! end directive is optional

!$omp parallel
block
   ...
end block
...
!$omp end parallel  !! error, considered as not match to above parallel directive

The parsing loop in parse_omp_structured_block() has been modified to allow
a BLOCK construct after the first statement has been detected to be ST_BLOCK.
This is done by a hard modification of the state into (the new) COMP_OMP_STRICTLY_STRUCTURED_BLOCK
after the statement is known (I'm not sure if there's a way to 'peek' the next
statement/token in the Fortran FE, open to suggestions on how to better write this)

Tested with no regressions on trunk, is this okay to commit?

Thanks,
Chung-Lin

2021-10-07  Chung-Lin Tang  <cltang@codesourcery.com>

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): Adjust declaration, add
	'bool strictly_structured_block' default true parameter, 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/strictly-structured-block-1.f90: New test.
  

Hi Chung-Lin,

On 07.10.21 15:59, Chung-Lin Tang wrote:
> this patch add support for "strictly-structured blocks" introduced in
> OpenMP 5.1,
> basically allowing BLOCK constructs to serve as the body for directives:
>
> !$omp target
> block
>   ...
> end block
> [!$omp end target]  !! end directive is optional

Pre-remark: That OpenMP feature causes ambiguities.
I have filled an OpenMP spec issue to discuss this, Issue 3154.

Namely the following is unclear:

!$omp parallel
  !$omp parallel
   block
     x=  x+  1
   end block
  !$omp end parallel

Does the 'end parallel' end the inner strictly structured block
or the outer loosely structured block?

In principle, a compiler could defer this until later during
parsing, but this requires a tremendous state tracking and it
is surely not simple to do in gfortran (and probably all other
compilers). — Thus, I think the spec will change and probably
in the way which this patch has implemented.

NOTE: It takes the kind of directive into account, i.e.
'omp end target' vs. 'omp end parallel' aren't ambiguous.


> The parsing loop in parse_omp_structured_block() has been modified to
> allow
> a BLOCK construct after the first statement has been detected to be
> ST_BLOCK.
> This is done by a hard modification of the state into (the new)
> COMP_OMP_STRICTLY_STRUCTURED_BLOCK
> after the statement is known (I'm not sure if there's a way to 'peek'
> the next
> statement/token in the Fortran FE, open to suggestions on how to
> better write this)
>
> Tested with no regressions on trunk, is this okay to commit?

LGTM  – unless Jakub has further comments.

However, I have two requests:

First, can you include an update of the implementation status
in libgomp/libgomp.texi (+ fix the ...ppp... typo):

"@item Suppport of strictly structured blocks in Fortran @tab N @tab"


Secondly, can you extend the testcase a bit to include
nesting a BLOCK inside the other BLOCK – and nesting
the directive with a strictly structured block inside
another (different) directive to ensure that the
'omp end ...' is correctly matched.

I mean something like:

integer :: x
!$omp target map(i)
  !$omp parallel
   block
     block
      x = x + 1
     end block
   end block
!$omp end target

!$omp target map(i)
  !$omp parallel
   block
     block
      x = x + 1
     end block
   end block
   !$omp end parallel
!$omp end target
end

Thanks,

Tobias

> 2021-10-07 Chung-Lin Tang  <cltang@codesourcery.com>
>
> 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): Adjust declaration, add
>     'bool strictly_structured_block' default true parameter, 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/strictly-structured-block-1.f90: New test.
-----------------
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
  

On Thu, Oct 07, 2021 at 09:59:00PM +0800, Chung-Lin Tang wrote:
> this patch add support for "strictly-structured blocks" introduced in OpenMP 5.1,
> basically allowing BLOCK constructs to serve as the body for directives:
> 
> !$omp target
> block
>   ...
> end block
> [!$omp end target]  !! end directive is optional
> 
> !$omp parallel
> block
>   ...
> end block
> ...
> !$omp end parallel  !! error, considered as not match to above parallel directive
> 
> The parsing loop in parse_omp_structured_block() has been modified to allow
> a BLOCK construct after the first statement has been detected to be ST_BLOCK.
> This is done by a hard modification of the state into (the new) COMP_OMP_STRICTLY_STRUCTURED_BLOCK
> after the statement is known (I'm not sure if there's a way to 'peek' the next
> statement/token in the Fortran FE, open to suggestions on how to better write this)

Thanks for working on this.
The workshare/parallel workshare case is unclear, I've filed
https://github.com/OpenMP/spec/issues/3153
for it.  Either don't allow block if workshare_stmts_only for now
until that is clarified, or if we do, we need to make sure that it does the
expected thing, does that gfc_trans_block_construct call ensure it?

Then we have the
https://github.com/OpenMP/spec/issues/3154
issue Tobias discovered, if that issue is resolved to end always applying to
the directive before the block statement, I think your patch handles it that
way but we want testsuite coverage for some of those cases.

For the testcases, I think best would be to split it into two, one that
contains only what we want to accept and another one with dg-errors in it.

I don't think the patch does the right thing for sections/parallel sections.
That is (at least in 5.1) defined as:
!$omp sections clauses...
[!$omp section]
structured-block-sequence
[!$omp section
structured-block-sequence]
...
!$omp end sections
(and similarly for parallel sections).
I believe your patch properly disallows:
!$omp sections
block
...
!$omp section
...
end block
!$omp end sections
- block itself is allowed, e.g.
!$omp sections
block
a=1
b=2
end block
!$omp end sections
with the meaning that the block is after the first implied !$omp section
and there is nothing else.  But does the patch actually check that
!$omp sections
block
...
end block
c=1
!$omp end sections
or
!$omp sections
!$omp section
block
...
end block
c=1
!$omp section
d=1
!$omp end sections
is invalid?  Though, not sure if that was the intended effect, in
OpenMP 5.0 that used to be fine.  But then the other changes are backwards
incompatible too,
!$omp parallel
block
  ...
end block
c=1
!$omp end parallel
used to be valid but no longer is.  structured-block-sequence is fortran
defined as structured-block and structured-block is defined as either
loosely-structured-block or strictly-structured-block, so for sections
in between each !$omp section should be either anything not starting with
block, or, if it starts with block, after end block there should be
immediately !$omp section or !$omp end {,parallel }sections.

Another thing is scan, the wording is similar and newly
!$omp do reduction(+,inscan:a)
do i=1,10
block
  ...
end block
x=1
!$omp scan
block
  ...
end block
x=2
end do
is invalid.

> @@ -5538,6 +5539,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;

The usual coding conventions put * before variable name instead of after it
(except for libstdc++).

> --- /dev/null
> +++ b/gcc/testsuite/gfortran.dg/gomp/strictly-structured-block-1.f90
> @@ -0,0 +1,295 @@
> +! { dg-do compile }
> +! { dg-options "-fopenmp" }
> +
> +program main
> +  integer :: x
> +
> +  !$omp parallel
> +  block
> +    x = x + 1
> +  end block
> +
> +  !$omp parallel
> +  block
> +    x = x + 1
> +  end block
> +  !$omp end parallel
> +
> +  !$omp parallel
> +  block
> +    x = x + 1
> +  end block
> +  x = x + 1
> +  !$omp end parallel ! { dg-error "Unexpected !.OMP END PARALLEL statement" }

Other than the splitting into non-dg-error stuff in one testcase and
dg-error in another one, I think we want (probably in yet another pair of
testcases) test what we are not required to do but with your patch we
actually implement, in particular that !$omp master behaves the same way.

> +  !$omp ordered
> +  block
> +    x = x + 1
> +  end block
> +
> +  !$omp ordered
> +  block
> +    x = x + 1
> +  end block
> +  !$omp end ordered
> +
> +  !$omp ordered
> +  block
> +    x = x + 1
> +  end block
> +  x = x + 1
> +  !$omp end ordered ! { dg-error "Unexpected !.OMP END ORDERED statement" }

I believe these 3 can't be done in the program, would either need to be
wrapped in some !$omp do with ordered clause or should go each ordered into
its own orphaned subroutine, because ordered region must bind to a
worksharing-loop with ordered clause.  I think wrapping it inside of !$omp
do ordered is easier.

	Jakub
  

On Thu, Oct 07, 2021 at 07:09:07PM +0200, Jakub Jelinek wrote:
> The workshare/parallel workshare case is unclear, I've filed
> https://github.com/OpenMP/spec/issues/3153
> for it.  Either don't allow block if workshare_stmts_only for now
> until that is clarified, or if we do, we need to make sure that it does the
> expected thing, does that gfc_trans_block_construct call ensure it?
> 
> Then we have the
> https://github.com/OpenMP/spec/issues/3154
> issue Tobias discovered, if that issue is resolved to end always applying to
> the directive before the block statement, I think your patch handles it that
> way but we want testsuite coverage for some of those cases.

Just want to follow-up on this, we now have resolutions of the
https://github.com/OpenMP/spec/issues/3153
https://github.com/OpenMP/spec/issues/3154
https://github.com/OpenMP/spec/issues/3155
issues and we can use that to guide this patch.
BLOCK is now explicitly allowed for workshare around the body of
workshare/parallel workshare or around the body of critical in it but not
arbitrarily nested.  My understanding of the patch is that it most likely
implements that, just we need a testsuite coverage that
!$omp workshare
block
  a = 1
  b = 2
  !$omp critical
  block
    c = 3
  end block
end block
is fine (also with !$omp end {criticial,workshare} after the block), but
that
!$omp workshare
a = 1
block
  b = 2
  c = 3
end block
!$omp end workshare
etc. is diagnosed.
For Tobias' issue that !$omp end whatever after end block for strictly
structured block binds to the directive above the strictly structured block
I think the patch also implements it but we want again testsuite coverage,
that
subroutine foo
!$omp parallel
!$omp parallel
block
end block
!$omp end parallel
!$omp end parallel
end subroutine foo
subroutine bar
!$omp teams
!$omp parallel
block
end block
!$omp end teams
end subroutine bar
is fine while e.g.
subroutine baz
!$omp parallel
!$omp parallel
block
end block
!$omp end parallel
end subroutine baz
is not (!$omp end parallel pairs with the inner parallel rather than outer,
and the outer parallel's body doesn't start with BLOCK, so needs to be
paired with its !$omp end parallel).
And lastly, the 3rd ticket clarifies that for the separating directives
for Fortran basically the 5.0 state remains except that the body can be now
also optionally wrapped in a single BLOCK.
(And for C/C++ allows no statements at all in between the separating
directives or after/before them but still requires the {}s around it like
5.1 and earlier.  Here we implement the 5.1 wording and let's stay with
that.)
Thinking more about the Fortran case for !$omp sections, there is an
ambiguity.
!$omp sections
block
  !$omp section
end block
is clear and !$omp end sections is optional, but
!$omp sections
block
end block
is ambiguous during parsing, it could be either followed by !$omp section
and then the BLOCK would be first section, or by !$omp end sections and then
it would be clearly the whole sections, with first section being empty
inside of the block, or if it is followed by something else, it is
ambiguous whether the block ... end block is part of the first section,
followed by something and then we should be looking later for either
!$omp section or !$omp end section to prove that, or if
!$omp sections
block
end block
was the whole sections construct and we shouldn't await anything further.
I'm afraid back to the drawing board.

	Jakub
  

On Thu, Oct 14, 2021 at 12:20:51PM +0200, Jakub Jelinek via Gcc-patches wrote:
> Thinking more about the Fortran case for !$omp sections, there is an
> ambiguity.
> !$omp sections
> block
>   !$omp section
> end block
> is clear and !$omp end sections is optional, but
> !$omp sections
> block
> end block
> is ambiguous during parsing, it could be either followed by !$omp section
> and then the BLOCK would be first section, or by !$omp end sections and then
> it would be clearly the whole sections, with first section being empty
> inside of the block, or if it is followed by something else, it is
> ambiguous whether the block ... end block is part of the first section,
> followed by something and then we should be looking later for either
> !$omp section or !$omp end section to prove that, or if
> !$omp sections
> block
> end block
> was the whole sections construct and we shouldn't await anything further.
> I'm afraid back to the drawing board.

And I have to correct myself, there is no ambiguity in 5.2 here,
the important fact is hidden in sections/parallel sections being
block-associated constructs.  That means the body of the whole construct
has to be a structured-block, and by the 5.1+ definition of Fortran
structured block, it is either block ... end block or something that
doesn't start with block.
So,
!$omp sections
block
end block
a = 1
is only ambiguous in whether it is actually
!$omp sections
block
  !$omp section
end block
a = 1
or
!$omp sections
!$omp section
block
end block
!$omp end sections
a = 1
but both actually do the same thing, work roughly as !$omp single.
If one wants block statement as first in structured-block-sequence
of the first section, followed by either some further statements
or by other sections, then one needs to write
!$omp sections
!$omp section
block
end block
a = 1
...
!$omp end sections
or
!$omp sections
block
  block
  end block
  a = 1
...
end block

Your patch probably already handles it that way, but we again need
testsuite coverage to prove it is handled the way it should in all these
cases (and that we diagnose what is invalid).

	Jakub
  

On 2021/10/14 7:19 PM, Jakub Jelinek wrote:
> On Thu, Oct 14, 2021 at 12:20:51PM +0200, Jakub Jelinek via Gcc-patches wrote:
>> Thinking more about the Fortran case for !$omp sections, there is an
>> ambiguity.
>> !$omp sections
>> block
>>    !$omp section
>> end block
>> is clear and !$omp end sections is optional, but
>> !$omp sections
>> block
>> end block
>> is ambiguous during parsing, it could be either followed by !$omp section
>> and then the BLOCK would be first section, or by !$omp end sections and then
>> it would be clearly the whole sections, with first section being empty
>> inside of the block, or if it is followed by something else, it is
>> ambiguous whether the block ... end block is part of the first section,
>> followed by something and then we should be looking later for either
>> !$omp section or !$omp end section to prove that, or if
>> !$omp sections
>> block
>> end block
>> was the whole sections construct and we shouldn't await anything further.
>> I'm afraid back to the drawing board.
> 
> And I have to correct myself, there is no ambiguity in 5.2 here,
> the important fact is hidden in sections/parallel sections being
> block-associated constructs.  That means the body of the whole construct
> has to be a structured-block, and by the 5.1+ definition of Fortran
> structured block, it is either block ... end block or something that
> doesn't start with block.
> So,
> !$omp sections
> block
> end block
> a = 1
> is only ambiguous in whether it is actually
> !$omp sections
> block
>    !$omp section
> end block
> a = 1
> or
> !$omp sections
> !$omp section
> block
> end block
> !$omp end sections
> a = 1
> but both actually do the same thing, work roughly as !$omp single.
> If one wants block statement as first in structured-block-sequence
> of the first section, followed by either some further statements
> or by other sections, then one needs to write
> !$omp sections
> !$omp section
> block
> end block
> a = 1
> ...
> !$omp end sections
> or
> !$omp sections
> block
>    block
>    end block
>    a = 1
> ...
> end block
> 
> Your patch probably already handles it that way, but we again need
> testsuite coverage to prove it is handled the way it should in all these
> cases (and that we diagnose what is invalid).

The patch currently does not allow strictly-structured BLOCK for sections/parallel sections,
since I was referencing the 5.1 spec while writing it, although that is trivially fixable.
(was sensing a bit odd why those two constructs had to be specially treated in 5.1 anyways)

The bigger issue is that under the current way the patch is written, the statements inside
a [parallel] sections construct are parsed automatically by parse_executable(), so to enforce
the specified meaning of "structured-block-sequence" (i.e. BLOCK or non-BLOCK starting sequence of stmts)
will probably be more a bit harder to implement:

!$omp sections
block
    !$omp section
    block
      x=0
    end block
    x=1           !! This is allowed now, though should be wrong spec-wise
    !$omp section
    x=2
end block

Currently "$!omp section" acts essentially as a top-level separator within a sections-construct,
rather than a structured directive. Though I would kind of argue this is actually better to use for the
user (why prohibit what looks like very apparent meaning of the program?)

So Jakub, my question for this is, is this current state okay? Or must we implement the spec pedantically?

As for the other issues:
(1) BLOCK/END BLOCK is not generally handled in parse_omp_structured_block, so for workshare,
     it is only handled for the top-level construct, not within workshare. I think this is what you meant
     in the last mail.

(2) As for the dangling-!$omp_end issue Tobias raised, because we are basically using 1-statement lookahead,
     any "!$omp end <*>" is naturally bound with the adjacent BLOCK/END BLOCK, so we should be okay there.

Thanks,
Chung-Lin
  

On Sat, Oct 16, 2021 at 02:44:12AM +0800, Chung-Lin Tang wrote:
> The patch currently does not allow strictly-structured BLOCK for sections/parallel sections,
> since I was referencing the 5.1 spec while writing it, although that is trivially fixable.
> (was sensing a bit odd why those two constructs had to be specially treated in 5.1 anyways)
> 
> The bigger issue is that under the current way the patch is written, the statements inside
> a [parallel] sections construct are parsed automatically by parse_executable(), so to enforce
> the specified meaning of "structured-block-sequence" (i.e. BLOCK or non-BLOCK starting sequence of stmts)
> will probably be more a bit harder to implement:
> 
> !$omp sections
> block
>    !$omp section
>    block
>      x=0
>    end block
>    x=1           !! This is allowed now, though should be wrong spec-wise
>    !$omp section
>    x=2
> end block
> 
> Currently "$!omp section" acts essentially as a top-level separator within a sections-construct,
> rather than a structured directive. Though I would kind of argue this is actually better to use for the
> user (why prohibit what looks like very apparent meaning of the program?)
> 
> So Jakub, my question for this is, is this current state okay? Or must we implement the spec pedantically?

I'd certainly not implement 5.1 pedantically when we know we'd change one
way for 5.0 -> 5.1 and change it back again for 5.1 -> 5.2.
An example of that is
!$omp sections
!$omp section
block
end block
x = 1
!$end omp sections
This is valid in 5.0 and will be valid again in 5.2 with the same meaning,
so let's just use the 5.0/5.2 wording here.  Ditto the !$omp end ambiguity
Tobias raised etc.
Whether to add support for the 5.2 behavior when one sees
!$omp {,parallel }sections
block
or not is more tough question, I bet the answer should be what is easier to
implement right now (i.e. don't spend too much effort on hard 5.1
implementation if 5.2 would be easier, as eventually we'd need to do the 5.2
implementation afterwards anyway).
So, for block right after sections either we implement the 5.2 wording right
away and therefore look for !$omp section only within that block and not
outside of it, or we for the 1st section with omitted !$omp section before
it only implement the 5.1 pedantic behavior, i.e. if it starts with a block,
don't look for !$omp section in the block, but require either !$omp section
or !$omp end sections right after the corresponding end block.

Does this answer all questions?

	Jakub