[Fortran] OpenMP+OpenACC: Remove bogus contigous-pointer check (PR/65438)

Permit more valid code by removing a too tight constraint – simple 
patch, very long background & history (feel free to skip).


Arrays in Fortran are classified as:

* assumed-size: "… :: var(*)" – and array but only the caller knows the 
size, which usually gets passed as some additional argument ("N") in the 
function all. – But at least contiguous. Typical for FORTRAN 66 code.
* assumed-rank: "… :: var(..)" – array can be any rank (dimension); 
provides full descriptor but the rank is not know at compile time, may 
or may not contigous/allocatable/pointer. Introduced mainly for MPI to 
avoid creating 16 versions of an interface (scalar to rank-15 arrays) – 
but has many uses; requires 'select rank' to be consumable within 
Fortran. — New with Fortran 2018 (or rather the TS29113, i.e. between 
F2013 and F2018).
* assumed shape: "… :: var(:, :)" – nonallocatable, nonpointer. Uses an 
array descriptor, can be contiguous or not (Always contiguous with 
Fortran 2008's 'contiguous' attribute)
*deferred shape: "var(:, :, :)" with allocatable/pointer attribute. 
'allocatable' implies contiguous by construction.
(* plus explicit-shape arrays like "… :: var(N,M)"  and with constant 
initialization: implied-shape arrays.)


openmp.c's "check_array_not_assumed" shows an error for:
* assumed-size arrays  (makes totally sense)
* assumed-rank arrays (makes sense as long as TS29113/F2018 is not 
supported)
* pointers which do not have the "contiguous" attribute

Not only function name wise, the latter is the odd one out. While in 
many cases a contiguous memory is required, one can either make it a 
compile-time testable constraint ('simply contiguous') – or one puts the 
onus is on the programmer; the latter seems to be done in the 
OpenACC/OpenMP specs.

(Of cause, it is also run-time checkable and gfortran uses a run-time 
test when passing a potentially noncontiguous array to a dummy argument 
with "contiguous" attribute; if the actual argument is contiguous, it is 
passed as is - otherwise, a copy-in and copy-out is performed [for 
intent(inout)].)


Currently, OpenACC 2.7 has:

* Based on Fortran 2003 – i.e. no 'contiguous attribute'.
* "2.7.1. Data Specification in Data Clauses": "Any array or subarray in 
a data clause, including Fortran array pointers, must be a contiguous 
block of memory, except for dynamic multidimensional C arrays."
* "2.14.4. Update Directive": "Noncontiguous subarrays may be specified. 
It is implementation-specific whether noncontiguous regions are updated 
by using one transfer for each contiguous subregion, or whether the 
noncontiguous data is packed, transferred once, and unpacked, or whether 
one or more larger subarrays (no larger than the smallest contiguous 
region that contains the specified subarray) are updated."

Oddly enough, OpenACC 2.7 only refers to "Fortran 2003" (in 1.6) but 
uses a TS29113/F2018 feature in "3.2.20. acc_copy_in": assumed-rank arrays.


OpenMP – OpenMP 4.0 and 4.5 are based on Fortran 2003 (hence: no 
'contiguous' attribute), OpenMP 5.0 is based on Fortran 2008. Hence, it 
explicitly uses the contiguous attribute. It also introduces 'simply 
contiguous array section' which largely matches Fortran's simply 
contiguous.Several contiguous restrictions were lifted with OpenMP 5.0; 
one of the few remaining ones is:

* [OpenMP 5's map clause] "If a list item is an array section, it must 
specify contiguous storage"
But this also does not require (simply) contiguous, i.e. compile-time 
checking.


The current check is used by OpenACC in

* resolve_oacc_data_clauses
* resolve_oacc_deviceptr_clause
* resolve_omp_clauses for OpenACC loops/EXEC_OACC_PARALLEL

And by OpenMP in

* resolve_omp_clauses for OMP_LIST_USE_DEVICE/OMP_LIST_DEVICE_RESIDENT


Some more archeology (thanks Thomas!):
* PR fortran/65438 (of 2015-03-16) - requires to remove this check, 
without providing any reasoning
* https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg145081.html 
(of 2016-08-09) was an attempt to add a warning – which was rejected on 
a similar ground then listed above: The 'contiguous' attribute (or 
simply contiguous property) is not explicitly ask for in the spec – and 
as OpenACC only supports Fortran < 2008, enforcing a Fortran 2008 
feature does not make sense.


HENCE: Remove a compile-time check when both specs (OpenMP/OpenACC) put 
the onus on the programmer. As compile-time checks are only a subset, we 
reject valid contiguous memory which is just not simply contiguous.

Additionally, the check only works if only an identifier is permitted as 
argument (it checks the symbol). For derived-type components, the check 
is not effective. (Assumed rank and assumed size are properties of dummy 
arguments, hence, those work fine.)

OK for the trunk?

Cheers,

Tobias

PS: This patch comes from the OG9 branch as 
ac6c90812344f4f4cfe4d2f5901c1a9d038a4000 but was actually already in the 
OG8 branch with commit d50862a7522446cf101eac9dc2e32967dc8318b7 from 
2015 (!) – I have used the ChangeLog entry from the latter.

Hi Tobias,

What about using gfc_is_not_contigous? This would allow to issue an error when we can prove the user made an error.

Regards

Thomas

On Tue, Oct 15, 2019 at 11:42:12AM +0200, Tobias Burnus wrote:
> OpenMP – OpenMP 4.0 and 4.5 are based on Fortran 2003 (hence: no
> 'contiguous' attribute), OpenMP 5.0 is based on Fortran 2008. Hence, it
> explicitly uses the contiguous attribute. It also introduces 'simply
> contiguous array section' which largely matches Fortran's simply
> contiguous.Several contiguous restrictions were lifted with OpenMP 5.0; one
> of the few remaining ones is:
> 
> * [OpenMP 5's map clause] "If a list item is an array section, it must
> specify contiguous storage"
> But this also does not require (simply) contiguous, i.e. compile-time
> checking.

The function you are changing has been introduced with OpenACC and is used
solely for OpenACC clauses, so I'll defer review to Thomas here.

> The current check is used by OpenACC in
> 
> * resolve_oacc_data_clauses
> * resolve_oacc_deviceptr_clause
> * resolve_omp_clauses for OpenACC loops/EXEC_OACC_PARALLEL
> 
> And by OpenMP in
> 
> * resolve_omp_clauses for OMP_LIST_USE_DEVICE/OMP_LIST_DEVICE_RESIDENT

device_resident and use_device are OpenACC clauses,
resolve_omp_clauses is called for both OpenMP and OpenACC, but is called
only for OpenACC:
                if (code
                    && (oacc_is_loop (code) || code->op == EXEC_OACC_PARALLEL))
                  check_array_not_assumed (n->sym, n->where, name);
and for those 2 clauses.

	Jakub

Hi Thomas,

On 10/15/19 12:59 PM, Thomas König wrote:
> What about using gfc_is_not_contigous? This would allow to issue an error when we can prove the user made an error.

Most clauses take only an identifiers (i.e. "sym" not "expr"). The 
gfc_is_not_contiguous check only returns true if there is an explicit 
array reference with strides. – All current callees only have "sym" (or 
in one case an expr which is only an identifier).

However, it might be useful for the few places like OpenMP's "map" 
clause which also permit array section. I will try to remember this 
rather new function (added Jan 2019) when adding some constraint checks.

Thanks,

Tobias

Hi Tobias!

On 2019-10-15T11:42:12+0200, Tobias Burnus <tobias@codesourcery.com> wrote:
> Permit more valid code by removing a too tight constraint – simple 
> patch, very long background & history (feel free to skip).

Thanks for writing that down, that's helpful to have in the archives.
(Certainly helps me, learning Fortran piece by piece.)  ;-)

> openmp.c's "check_array_not_assumed" shows an error for:
> * assumed-size arrays  (makes totally sense)
> * assumed-rank arrays (makes sense as long as TS29113/F2018 is not 
> supported)
> * pointers which do not have the "contiguous" attribute
>
> Not only function name wise, the latter is the odd one out. While in 
> many cases a contiguous memory is required, one can either make it a 
> compile-time testable constraint ('simply contiguous') – or one puts the 
> onus is on the programmer; the latter seems to be done in the 
> OpenACC/OpenMP specs.
>
> (Of cause, it is also run-time checkable

OK, I was about to ask for that, if that makes sense to do.

> and gfortran uses a run-time 
> test when passing a potentially noncontiguous array to a dummy argument 
> with "contiguous" attribute; if the actual argument is contiguous, it is 
> passed as is - otherwise, a copy-in and copy-out is performed [for 
> intent(inout)].)

I read this such that what we've got is sufficient, no further run-time
checking is needed.

> HENCE: Remove a compile-time check when both specs (OpenMP/OpenACC) put 
> the onus on the programmer. As compile-time checks are only a subset, we 
> reject valid contiguous memory which is just not simply contiguous.

Is there any point in adding a test case for such constructs, in
particular, which the current code would've (erroneously) flagged as
"Noncontiguous deferred shape array", to codify/document that this is
supported?

> Additionally, the check only works if only an identifier is permitted as 
> argument (it checks the symbol). For derived-type components, the check 
> is not effective. (Assumed rank and assumed size are properties of dummy 
> arguments, hence, those work fine.)

> OK for the trunk?

I'll gladly defer ;-) to your Fortran expertise as well as OpenACC
specification interpretation.  (Test case, if feasible, pre-approved
anyway.)

> PS: This patch comes from the OG9 branch as 
> ac6c90812344f4f4cfe4d2f5901c1a9d038a4000 but was actually already in the 
> OG8 branch with commit d50862a7522446cf101eac9dc2e32967dc8318b7 from 
> 2015 (!)

(Just for the record, before that on gomp-4_0-branch, and before that in
an internal development branch.)  ;-)

> – I have used the ChangeLog entry from the latter.

ACK.  Given that you've now re-researched all the rationale, it's
certainly fine to add your name, too.  The ChangeLog date needs to be the
actual commit date.


Grüße
 Thomas

Hi Thomas,

On 10/15/19 3:07 PM, Thomas Schwinge wrote:
>> (Of cause, it is also run-time checkable
> OK, I was about to ask for that, if that makes sense to do.

The question is for what. One could add it for run-time checking (e.g. 
for gfortran "-fcheck=…). Or for actions like "omp update", which 
permits strides/noncontiguous memory.

But for "omp update" with noncontiguous memory, it is not clear what's 
faster:
* Simply updating one contiguous memory block, enclosing the gaps
* Doing multiple updates of contiguous memory to take advantage of 
memory gaps.

Depending how large the gaps between the (contiguous) chunks are, one 
method or the other is faster. Thus, I don't think we do need this any 
time soon. At least for the OpenMP code I have seen, some additional 
compile-time checks make more sense before adding a run-time check. [No 
idea about the OpenACC FE code, yet.]


> Is there any point in adding a test case for such constructs, in 
> particular, which the current code would've (erroneously) flagged 
> as"Noncontiguous deferred shape array", to codify/document that this 
> is supported? 

My feeling is no – but if you think it makes sense. A simple example is 
the following:

integer, target :: A(100)
integer, pointer :: ptr(:)
ptr => A
!$acc data copy(ptr)
ptr(1) = ptr(2)
!$acc end data
end

Before, it failed with:

     4 | !$acc data copy(ptr)
       |                1
Error: Noncontiguous deferred shape array ‘ptr’ in MAP clause at (1)


I will now commit it as is, i.e. without a test case. If needed, one can 
still add it as additional commit.

Tobias

On Tue, Oct 15, 2019 at 04:10:34PM +0200, Tobias Burnus wrote:
> But for "omp update" with noncontiguous memory, it is not clear what's
> faster:
> * Simply updating one contiguous memory block, enclosing the gaps
> * Doing multiple updates of contiguous memory to take advantage of memory
> gaps.

omp target update with non-contiguous array sections isn't supported yet
even for C/C++, and when it is, it would be nice to handle it efficiently.
We already have omp_target_memcpy_rect function that can copy non-contiguous
data, but again there is no plugin API to allow at least for the common
cases that some hw is able to handle (perhaps 2 or 3 dimensions).  Can at
least CUDA handle that?  AMD GCN?
Anyway, I'm afraid that is left for next year.

	Jakub