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<tob...@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 <tob...@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" } }