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

[Tobias Burnus <tobias at codesourcery dot com>]

Hi Thomas,

On 11/28/19 6:01 PM, Thomas Schwinge wrote:
> Definition (3.32.1 in F2018): "blank common" = "unnamed common block". 

I just want to add the following, which came into my mind after thinking 
more about device_resident (the other email in this thread). Fortran 
(here: 2018, 8.10.2.5) has:

"Named common blocks of the same name shall be of the same size in all 
scoping units of a program in which they appear, but blank common blocks 
may be of different sizes."

* * *

Depending on the use of a common block (see other email in the same 
thread, to be send shortly), that's fine or not. If the common block 
only exists on the device (i.e. in a device routine / 'target' 
procedure) or only on the host, everything is fine. — In this case, the 
connection between host and target is done by single variables – and no 
one cares whether they are in a common block or not.

It only becomes interesting if the same(-named) common block is known to 
both the host and the device – in that case, it is important that the 
size matches, otherwise either the copying to the device or (via 
'update') from the device to the host will write beyond the static 
variable! — Also in the latter case, it makes sense that 
'copy(/block_name/)' will map the whole common block and not only the 
directly used variables (which might be none).

* * *

OpenACC: Does one need device_resident to allocate 'static' global 
memory on the device? If not, then its only use would be for same-named 
common blocks, existing on both the device and the host. If it is 
needed, then one needs to think about the semantic – will it declare a 
common block which exists only on the device or one which exists on both 
device and host with the same name. — I think that needs to be spelt out 
in the spec clearly; at the moment, it is ambiguous. In any case, it 
influences how copy(/block_name/) acts.

For OpenMP, my impression is that the spec is completely silent on 
device-located common blocks. And if a common block is only on the host, 
copy(/block/) just maps the used (common-block) variables to the target 
– which is fine. — Seems as if some spec work is needed as well.

* * *

> I go by the assumption that everything contained in the base
> languages of OpenACC/OpenMP ([…] C, C++, Fortran
> standards), should also work in an OpenACC/OpenMP context in a sensible
> manner […]
I concur.
> ACK. Instead of "burying" such things in long emails, I like to see 
> GCC PRs filed, which can then be actioned on individually.

Well, I think one first needs to understand what's supposed to be in the 
standard. Having said this, I have now filled – PR 92728 + PR 92730.

[blank commons]

> assumption would thus be: yes, ought to be supported -- but I haven't 
> thought through whether that makes sense, so...

By itself, using blank commons make sense if one maps variables from a 
common block but not if one maps the whole common block.

[Blank commons + PGI]

I will later play around with the PGI compiler; but I think it is really 
a spec issue and I care less what a specific compiler does. (Even 
though, with OpenACC, it is kind of the reference compiler.)

> determine whether that makes sense to support in an OpenACC context

I think that needs discussion about what one wants to achieve instead of 
directly patching the spec.

> > > > * I have now a new test case
> > > > libgomp/testsuite/libgomp.oacc-fortran/common-block-3.f90 which looks at
> > > > omplower.
Actually, I think this should be: 
gcc/testsuite/gfortran.dg/goacc/common-block-3.f90
> > > Thanks. Curious: why 'omplower' instead of 'gimple' dump?
> > […]
> My rationale is that your code changes are in 'gcc/gimplify.c', so you'd
> test for that stuff in the 'gimple' dump (which is between 'original' and
> 'omplower').
[switching to gimple]
> I think it does (but please argue if it doesn't to you), but that's not
> high priority, of course.

Hmm, that might be more specific to other parts – but with optional 
arguments, I had constantly to look at what has been passed on to 
libgomp via omp_arr – even though the code was produced directly by the 
front end. The 'pragma' simply didn't tell the whole story – omp-low.c 
added and removed some '*' and '&' which were crucial.

Probably, 'map' as parsed here doesn't change any more between 
gimplify.c and the early stages of omp-low.c, but I feel safer if the 
wanted result survived until the end of omp-low.c and does not get 
modified in an unintended way later on.

> Note that I said 'dg-additional-options', not 'dg-options', so please
> re-consider.

Ups. Yes, dg-additional-options should work :-)

[oacc/pr84963.f90]

> Good find. […] to change 'dg-options "-O2"' to 'dg-additional-options 
> "-O2"
> Please verify, and then commit that to trunk, gcc-8-branch, gcc-7-branch, 

Done so – except I committed to GCC 9 instead of GCC 7, which is now 
closed :-)
[Question about a test case]

> Then let's please document that in the test case sources, for that's not
> quite obvious.

I have now committed the test-case patch with a comment as suggested: 
r278843.

Tobias