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

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Thomas König
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
Reply | Threaded
Open this post in threaded view
|

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

Jakub Jelinek
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
Reply | Threaded
Open this post in threaded view
|

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

Tobias Burnus-3
In reply to this post by Thomas König
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

Reply | Threaded
Open this post in threaded view
|

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

Thomas Schwinge-8
In reply to this post by Thomas König
Hi Tobias!

On 2019-10-15T11:42:12+0200, Tobias Burnus <[hidden email]> 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

signature.asc (671 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Tobias Burnus-5
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

Reply | Threaded
Open this post in threaded view
|

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

Jakub Jelinek
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