Re: [Patch][Fortran] OpenACC – permit common blocks in some clauses

[Tobias Burnus <tobias at codesourcery dot com>]

Hi Thomas,

sorry for the belated reply.

Some comments – and a patch modifying two test cases (see below).
Regarding the patch: OK for the trunk?

On 11/11/19 10:39 AM, Thomas Schwinge wrote:
> By the way, do you know what's the status is for Fortran common blocks in
> OpenMP: supported vs. expected per the specification?

No; however, I had a quick glance at the spec and at the test cases; 
both compile-time and run-time test have some coverage, although I 
didn't spot a run-time test for one 'omp target'.

Definition (3.32.1 in F2018): "blank common" = "unnamed common block". 
'common /name/ x" (continues) define the common block named "name" by 
adding 'x' to it. While "common // y" or "common y" appends 'y' to the 
blank common.

In OpenMP 5, common blocks appear twice – once [2.1, p.39, ll.11ff.] as 
general rule in the definition of "list item" (which are inherited by 
"extended list item" and "locator-list item"). [There are also some 
constraints and notes regarding common blocks)]. It does not really tell 
whether blank commons are permitted or not; some description is 
explicitly for named-common variables, leaving blank-common ones out 
(and undefined). But later sections explicitly make reference to blank 
commons, hence, one can assume they are permitted unless explicitly 
stated that they are not.

And then very selectively for some items:
* allocate – only with default allocator.
* declare target – some restrictions and no blank commons
* depend clause – no common permitted
* threadprivate – some notes and explanation of the syntax (why?)
  also only here requirement regarding common blocks with bind(c)
  (why not also for declare target?)
* linear clause – no common permitted
* copyin – some notes
* copyprivate – some notes

As target test cases were suspiciously left out, I tries '!$omp target 
map(/name/)' which was rejected. I think one should add test cases for 
newer features – which mostly means 'omp target' and add the missing 
common-block checks. – And one has to find out why blank commons are not 
permitted and for the places where they are permitted, support has to be 
added.

Talking about blank common blocks, the current OpenACC implementation 
does not seem to like them (see below); the spec (2.7) does not mention 
blank common blocks at all. – It talks about name between two slashes, 
but leaves it open whether the name can also be an empty string.

common // x,y  !blank common
!$acc parallel copyin(//)
!$acc end parallel
end

fails with:

    2 | !$acc parallel copyin(//)
      |                       1
Error: Syntax error in OpenMP variable list at (1)


On 2019-10-25T16:36:10+0200, Tobias Burnus<tobias@codesourcery.com>  wrote:

> > * I have now a new test case
> > libgomp/testsuite/libgomp.oacc-fortran/common-block-3.f90 which looks at
> > omplower.
> Thanks. Curious: why 'omplower' instead of 'gimple' dump?

So far I found -fdump-tree-original, -fdump-omplower and 
-fdump-optimized quite useful – I have so far not used 
-fdump-tree-gimple, hence, I cannot state what's the advantage of the 
latter.

The original dump I like because it shows what the FE generates, the 
omplower dump has the result after lowering including the assignments to 
the omp_arr variables but it keeps a readable pragma line (avoids 
guessing what the kind value was again etc.) while the optimized dump 
really shows what ends up in the call (with the pro and con that it 
depends on the optimization option).

If you think it makes sense, one can switch.

> > --- /dev/null
> > +++ b/gcc/testsuite/gfortran.dg/goacc/common-block-3.f90
> > @@ -0,0 +1,39 @@
> > +! { dg-options "-fopenacc -fdump-tree-omplower" }
> (For later: we usually just use 'dg-additional-options
> "-fdump-tree-omplower"'; '-fopenacc' is implied inside '*/goacc/'.)

My impression was that it is not effective unless repeated – and I think 
I even tries it. In gcc/testsuite/gfortran.dg/gomp/ all 64 test cases 
with dg-options specify re-add the "-fopenmp".

And in gcc/testsuite/gfortran.dg/goacc, 4 test cases use dg-options, 3 
specify -fopenacc and one doesn't. I wouldn't call this 'usually' and I 
wonder whether -fopenacc is really active for goacc/pr84963.f90. (The 
file uses no directive at all!)

Hence, I wonder whether one should add it to goacc/pr84963.f90 – see 
attached patch.

Without the patch, I get:

Executing on host: …/gfortran/../../gfortran -B…/gfortran/../../ 
-B…/./libgfortran/ …/goacc/pr84963.f90 -fno-diagnostics-show-caret 
-fno-diagnostics-show-line-numbers -fdiagnostics-color=never  
-fdiagnostics-urls=never    -O  -O2 -S -o pr84963.s    (timeout = 300)

And with the patch, I get
… -fdiagnostics-urls=never -O -fopenacc -O2 -S -o pr84963.s …


> > +  integer :: i, j
> > +  real ::  a(n) = 0, b(n) = 0, c, d
> > +  real ::  x(n) = 0, y(n), z
> > +  common /BLOCK/ a, b, c, j, d
> > +  common /KERNELS_BLOCK/ x, y, z
> For my understanding: the several unused variables in the common blocks
> are to make sure that they don't cause any issues, don't get mapped at
> all?

I think that's the idea – common-block variables which are not used 
should also not get mapped (= optimization). But, obviously, they should 
also not cause any issues.

Hence, one could/should also check that they are not mapped – done in 
the attached patch.

> I we were to add to 'gfortran.dg/goacc/common-block-3.f90' a test case
> for the upcoming OpenACC 'serial' construct (which basically equals the
> OpenACC 'parallel' construct), would we copy/adapt the 'parallel' 'BLOCK'
> test case, or add a new, separate common block?
>
> Or, asking the other way round: why aren't in the current test case,
> 'parallel' and 'kernels' using the same common block, and both explicitly
> 'copy' the common block vs. not do that?

I think one could do either way – by itself, the blocks should be 
independent and, hence, could re-use the same common block. Re-using the 
same common block tests other things, hence, maybe you should do so – 
and use different variables from both blocks.

> > * In the compile-time *{2,3} test case, there is now also a 'enter data'
> > and 'update host/self/device' test.
> ;-) Heh, 'update' got inside the 'parallel' region:
>
> > --- /dev/null
> > +++ b/gcc/testsuite/gfortran.dg/goacc/common-block-1.f90
> > @@ -0,0 +1,74 @@
> > +[...]
> > +  !$acc parallel firstprivate(/blockA/, /blockB/, e, v)
> > +  !$acc update device(/blockA/)
> > +  !$acc update self(/blockB/, v)
> > +  !$acc update host(/blockA/, e, /blockB/)
> > +  !$acc end parallel
> > +[...]
> > --- /dev/null
> > +++ b/gcc/testsuite/gfortran.dg/goacc/common-block-2.f90
> Likewise.
>
> As obvoius; see attached, committed "Fix OpenACC directives nesting in
> 'gfortran.dg/goacc/common-block-1.f90',
> 'gfortran.dg/goacc/common-block-2.f90'" to trunk in r278047.

Thanks.

Cheers,

Tobias

2019-11-25  Tobias Burnus  <tobias@codesourcery.com>

	* gfortran.dg/goacc/pr84963.f90: Add -fopenacc to dg-options.
	* gfortran.dg/goacc/common-block-3.f90: Check that unused common-block
	variables do not get mapped.

diff --git a/gcc/testsuite/gfortran.dg/goacc/pr84963.f90 b/gcc/testsuite/gfortran.dg/goacc/pr84963.f90
index 4548082bee3..aade95c1986 100644
--- a/gcc/testsuite/gfortran.dg/goacc/pr84963.f90
+++ b/gcc/testsuite/gfortran.dg/goacc/pr84963.f90
@@ -1,5 +1,5 @@
 ! PR ipa/84963
-! { dg-options "-O2" }
+! { dg-options "-fopenacc -O2" }
 
 program p
    print *, sin([1.0, 2.0])
diff --git a/gcc/testsuite/gfortran.dg/goacc/common-block-3.f90 b/gcc/testsuite/gfortran.dg/goacc/common-block-3.f90
index 9032d9331f0..c176e53f959 100644
--- a/gcc/testsuite/gfortran.dg/goacc/common-block-3.f90
+++ b/gcc/testsuite/gfortran.dg/goacc/common-block-3.f90
@@ -9,7 +9,7 @@ program main
   implicit none
 
   integer :: i, j
-  real ::  a(n) = 0, b(n) = 0, c, d
+  real ::  a(n) = 0, b(n) = 0, c, d, e(n)
   real ::  x(n) = 0, y(n), z
   common /BLOCK/ a, b, c, j, d
   common /KERNELS_BLOCK/ x, y, z
@@ -35,5 +35,8 @@ end program main
 ! { dg-final { scan-tree-dump-times "omp target oacc_kernels .*map\\(tofrom:y \\\[len: 400\\\]\\\)" 1 "omplower" } }
 ! { dg-final { scan-tree-dump-times "omp target oacc_kernels .*map\\(force_tofrom:c \\\[len: 4\\\]\\)" 1 "omplower" } }
 
-! { dg-final { scan-tree-dump-not "map\\(.*:block\\)" "omplower" } }
-! { dg-final { scan-tree-dump-not "map\\(.*:kernels_block\\)" "omplower" } }
+! { dg-final { scan-tree-dump-not "map\\(.*:block" "omplower" } }
+! { dg-final { scan-tree-dump-not "map\\(.*:kernels_block" "omplower" } }
+! { dg-final { scan-tree-dump-not "map\\(.*:d " "omplower" } }
+! { dg-final { scan-tree-dump-not "map\\(.*:e " "omplower" } }
+! { dg-final { scan-tree-dump-not "map\\(.*:z " "omplower" } }