[Bug fortran/62131] New: Openmp Regression 4.9.1 : Subobject of an allocatable array not allowed in OMP ATOMIC

[Bug fortran/62131] New: Openmp Regression 4.9.1 : Subobject of an allocatable array not allowed in OMP ATOMIC

[vladimir.fuka at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62131

            Bug ID: 62131
           Summary: Openmp Regression 4.9.1 : Subobject of an allocatable
                    array not allowed in OMP ATOMIC
           Product: gcc
           Version: 4.9.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: fortran
          Assignee: unassigned at gcc dot gnu.org
          Reporter: vladimir.fuka at gmail dot com

layzrc raised this problem on comp.lang.fortran
https://groups.google.com/forum/#!topic/comp.lang.fortran/lVzeHW_X1aw

module storage
  integer,allocatable :: nerrs(:,:)
end module

program atomic
  use storage
    allocate(nerrs(10,10))
!$omp parallel do
    do k=1,10
      call uperrs(k,1)
    enddo
!$omp end parallel do
  stop
contains
  subroutine uperrs(i,io)
  integer,intent(in) :: i,io
!$omp atomic
    nerrs(i,io)=nerrs(i,io)+1
  end subroutine
end

>gfortran -fopenmp atomic.f90
atomic.f90:18.29:

    nerrs(i,io)=nerrs(i,io)+1
                             1
Error: !$OMP ATOMIC with ALLOCATABLE variable at (1)



I believe this is a  wrong and causes a bad regression, because `nerrs(i,io)`
is not an allocatable variable. I believe the section 2.12.6 of OpenMP 4
concerns situations when the whole variable being updated or assigned is
allocatable (e.g. allocatable scalars) and it causes possible re-allocation.

[Bug fortran/62131] [4.9.1 Regression] OpenMP: Subobject of an allocatable array not allowed in OMP ATOMIC

[burnus at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62131

Tobias Burnus <burnus at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |openmp, rejects-valid
                 CC|                            |burnus at gcc dot gnu.org,
                   |                            |jakub at gcc dot gnu.org
   Target Milestone|---                         |4.9.3
            Summary|Openmp Regression 4.9.1 :   |[4.9.1 Regression] OpenMP:
                   |Subobject of an allocatable |Subobject of an allocatable
                   |array not allowed in OMP    |array not allowed in OMP
                   |ATOMIC                      |ATOMIC

--- Comment #1 from Tobias Burnus <burnus at gcc dot gnu.org> ---
To summon up: It's not about "!$OMP ATOMIC with ALLOCATABLE variable at (1)"
being the wrong check - it's about having a warning for a nonallocatable
variable.


The point is: Even if "a" is allocatable, "a(1,2)" is not allocatable - and for
"dt%b" it would depend on "b" and not on "a".

I think the OpenMP constraint is there to avoid that (re)allocation on
assignment gets triggered for atomic assignments.


The current code, cf. line 2744 of the gcc/fortran/openmp.c, uses:
  var = code->expr1->symtree->n.sym;
...
  if (var->attr.allocatable)

I think the simplest would be to use:
   if (gfc_expr_attr (code->expr1).allocatable))


However, there might be other spots in the code where using "var" looks at the
wrong thing for component references (like: "var%comp"). For instance,
              || expr2_tmp->symtree->n.sym == var)
might reject  "var%b = var%a" code - which may or may not be intended.

[Bug fortran/62131] [4.9/5 Regression] OpenMP: Subobject of an allocatable array not allowed in OMP ATOMIC

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62131

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2014-08-14
   Target Milestone|4.9.3                       |4.9.2
            Summary|[4.9.1 Regression] OpenMP:  |[4.9/5 Regression] OpenMP:
                   |Subobject of an allocatable |Subobject of an allocatable
                   |array not allowed in OMP    |array not allowed in OMP
                   |ATOMIC                      |ATOMIC
     Ever confirmed|0                           |1

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Tobias Burnus from comment #1)
> The current code, cf. line 2744 of the gcc/fortran/openmp.c, uses:
>   var = code->expr1->symtree->n.sym;
> ...
>   if (var->attr.allocatable)
> 
> I think the simplest would be to use:
>    if (gfc_expr_attr (code->expr1).allocatable))

That looks good to me, preapproved for trunk/4.9.2 with a testcase.

> However, there might be other spots in the code where using "var" looks at
> the wrong thing for component references (like: "var%comp"). For instance,
>               || expr2_tmp->symtree->n.sym == var)
> might reject  "var%b = var%a" code - which may or may not be intended.

Fix that is non-easy though.
var is used e.g. for the expr_references_sym checks.
Some of those are used to complain about invalid code, but if
expr1->ref is non-NULL, then we may reject valid code.
Say
!$omp atomic write
  var(1) = var(2)
is fine, yet we'll complain.  Those could be fixed by only calling
expr_references_sym if expr1->ref is NULL, and simply not diagnose it at all
otherwise.  Or expr_references_sym could be enhanced not to take symbol, but
the EXPR_VARIABLE gfc_expr * instead, and it could just try to compare the refs
too.  I believe in the spec when it talks about x, it should be always the same
sequence of tokens, so say
!$omp atomic
  var(i) = var(j) + 1
is likely invalid even if i has the same value as j, the standard requires that
no other expression may reference the same memory as x (etc.), but that is
pretty much impossible to check.
Anyway, bigger issue is the atomic swap (for which I'm guilty as the one who
requested the addition of that into the standard).
For that unfortunately the being conservative is bad, but the current state is
bad too.
!$omp atomic capture
  v = var(i)
  var(i) = var(j) + var(k)
!$omp end atomic
(of course, invalid with i == j or i == k) should be valid atomic swap that we
likely mishandle right now, while
!$omp atomic capture
  v = var(i)
  var(i) = var(j) + var(i)
!$omp end atomic
is not a capture.  So, I'm afraid we need to improve expr_references_sym.
And then there are the other comparisons you've mentioned, not sure what
exactly we can do about those :(.

[Bug fortran/62131] [4.9/5 Regression] OpenMP: Subobject of an allocatable array not allowed in OMP ATOMIC

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62131

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Author: jakub
Date: Fri Aug 15 10:23:13 2014
New Revision: 214010

URL: https://gcc.gnu.org/viewcvs?rev=214010&root=gcc&view=rev
Log:
    PR fortran/62131
    * openmp.c (resolve_omp_atomic): Only complain if code->expr1's attr
    is allocatable, rather than whenever var->attr.allocatable.

    * gfortran.dg/gomp/pr62131.f90: New test.

Added:
    trunk/gcc/testsuite/gfortran.dg/gomp/pr62131.f90
Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/openmp.c
    trunk/gcc/testsuite/ChangeLog

[Bug fortran/62131] [4.9/5 Regression] OpenMP: Subobject of an allocatable array not allowed in OMP ATOMIC

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62131

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Author: jakub
Date: Fri Aug 15 10:24:08 2014
New Revision: 214011

URL: https://gcc.gnu.org/viewcvs?rev=214011&root=gcc&view=rev
Log:
    PR fortran/62131
    * openmp.c (resolve_omp_atomic): Only complain if code->expr1's attr
    is allocatable, rather than whenever var->attr.allocatable.

    * gfortran.dg/gomp/pr62131.f90: New test.

Added:
    branches/gcc-4_9-branch/gcc/testsuite/gfortran.dg/gomp/pr62131.f90
Modified:
    branches/gcc-4_9-branch/gcc/fortran/ChangeLog
    branches/gcc-4_9-branch/gcc/fortran/openmp.c
    branches/gcc-4_9-branch/gcc/testsuite/ChangeLog

[Bug fortran/62131] [4.9/5 Regression] OpenMP: Subobject of an allocatable array not allowed in OMP ATOMIC

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62131

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
The OpenMP 4.1 wording seems to be to allow this.  Thus fixed.