[Bug fortran/97655] New: gcc/fortran/openmp.c:4133: possible cut'n'paste error ?

[Bug fortran/97655] New: gcc/fortran/openmp.c:4133: possible cut'n'paste error ?

[dcb314 at hotmail dot com]

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

            Bug ID: 97655
           Summary: gcc/fortran/openmp.c:4133: possible cut'n'paste error
                    ?
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: fortran
          Assignee: unassigned at gcc dot gnu.org
          Reporter: dcb314 at hotmail dot com
  Target Milestone: ---

gcc/fortran/openmp.c:4133:12: style: Expression is always false because 'else
if' condition matches previous condition at line 4131. [multiCondition]

Source code is

          if (c->atomic_op == GFC_OMP_ATOMIC_READ)
            c->memorder = OMP_MEMORDER_ACQUIRE;
          else if (c->atomic_op == GFC_OMP_ATOMIC_READ)
            c->memorder = OMP_MEMORDER_RELEASE;
          else
            c->memorder = OMP_MEMORDER_ACQ_REL;

[Bug fortran/97655] gcc/fortran/openmp.c:4133: possible cut'n'paste error ?

[dcb314 at hotmail dot com]

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

David Binderman <dcb314 at hotmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |burnus at gcc dot gnu.org

--- Comment #1 from David Binderman <dcb314 at hotmail dot com> ---
git blame says:

fc5e7ef98e1 (Tobias Burnus           2020-10-30 15:57:46 +0100 4131)     if
(c->atomic_op == GFC_OMP_ATOMIC_READ)
1fc5e7ef98e1 (Tobias Burnus           2020-10-30 15:57:46 +0100 4132)      
c->memorder = OMP_MEMORDER_ACQUIRE;
1fc5e7ef98e1 (Tobias Burnus           2020-10-30 15:57:46 +0100 4133)     else
if (c->atomic_op == GFC_OMP_ATOMIC_READ)
1fc5e7ef98e1 (Tobias Burnus           2020-10-30 15:57:46 +0100 4134)      
c->memorder = OMP_MEMORDER_RELEASE;
1fc5e7ef98e1 (Tobias Burnus           2020-10-30 15:57:46 +0100 4135)     else
1fc5e7ef98e1 (Tobias Burnus           2020-10-30 15:57:46 +0100 4136)      
c->memorder = OMP_MEMORDER_ACQ_REL;

Adding code author.

[Bug fortran/97655] gcc/fortran/openmp.c:4133: possible cut'n'paste error ?

[jakub at gcc dot gnu.org]

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

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Guess the second condition should be !c->capture.
The spec says that atomic_default_mem_order(acq_rel) makes mem-order-clause
default to acquire for read, acq_rel for capture and release otherwise - write
or update or atomic-clause missing.
Now, something I have clearly missed in the review, why is capture not part of
atomic_op?  capture is atomic-clause like the others.  And I'd be afraid we'd
allow something like !$omp atomic capture update or !$omp atomic update capture
by making it separate.

[Bug fortran/97655] gcc/fortran/openmp.c:4133: possible cut'n'paste error ?

[burnus at gcc dot gnu.org]

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

--- Comment #3 from Tobias Burnus <burnus at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #2)
> Guess the second condition should be !c->capture.

I concur.


> Now, something I have clearly missed in the review, why is capture not part
> of atomic_op?  capture is atomic-clause like the others.

Because of preparation/anticipation for OpenMP 5.1 (TR9), which has:
* atomic-clause is one of the following:
  read, write, update
* memory-order-clause is one of the following:
  seq_cst, acq_rel, release, acquire, relaxed
* clause is either atomic-clause, memory-order-clause or one of the following:
  capture, compare, hint(hint-expression), fail(seq_cst | acquire | relaxed),
  weak

With:
* If atomic-clause is not present on the construct, the behavior is as if the
update clause is specified.
* If a capture or compare clause appears on the construct then atomic-clause
  must be update.

> And I'd be afraid we'd allow something like !$omp atomic capture update or
> !$omp atomic update capture by making it separate.

Which is intended (for OpenMP 5.1). I did loop at OpenMP 5.0 + 5.1 and OpenACC
2.6 + 3.0, but I admittedly missed that 'capture' could not occur with 'update'
in OpenMP 5.0, also because the previous code was used for OpenACC, which does
permit 'update capture' besides 'capture' (since OpenACC 2.5).

[Bug fortran/97655] gcc/fortran/openmp.c:4133: possible cut'n'paste error ?

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Oh, missed that totally, seems atomics changed a lot in 5.1
So I think it is ok to handle them separately, but I'd strongly prefer to
ensure that we reject the forms invalid in 5.0 even if they will be valid in
5.1, at least until we get most of the 5.0 support done (perhaps sans
OMPD/OMPT).

[Bug fortran/97655] gcc/fortran/openmp.c:4133: possible cut'n'paste error ?

[marxin at gcc dot gnu.org]

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

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2020-11-02
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1

[Bug fortran/97655] gcc/fortran/openmp.c:4133: possible cut'n'paste error ?

[cvs-commit at gcc dot gnu.org]

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

--- Comment #5 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Tobias Burnus <burnus@gcc.gnu.org>:

https://gcc.gnu.org/g:b2a31e2c341d96520c5fb7c1e1f1c590eb182d7f

commit r11-4606-gb2a31e2c341d96520c5fb7c1e1f1c590eb182d7f
Author: Tobias Burnus <tobias@codesourcery.com>
Date:   Mon Nov 2 13:07:17 2020 +0100

    Fortran: OpenMP - fixes for omp atomic [PR97655]

    gcc/fortran/ChangeLog:

            PR fortran/97655
            * openmp.c (gfc_match_omp_atomic): Fix mem-order handling;
            reject specifying update + capture together.

    gcc/testsuite/ChangeLog:

            PR fortran/97655
            * gfortran.dg/gomp/atomic.f90: Update tree-dump counts; move
            invalid OMP 5.0 code to ...
            * gfortran.dg/gomp/atomic-2.f90: ... here; update dg-error.
            * gfortran.dg/gomp/requires-9.f90: Update tree dump scan.

[Bug fortran/97655] gcc/fortran/openmp.c:4133: possible cut'n'paste error ?

[burnus at gcc dot gnu.org]

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

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

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

--- Comment #6 from Tobias Burnus <burnus at gcc dot gnu.org> ---
(In reply to David Binderman from comment #0)
> gcc/fortran/openmp.c:4133:12: style: Expression is always false because
> 'else if' condition matches previous condition at line 4131. [multiCondition]

Thanks for the report – this issue has been FIXED as well as the other issues
discussed here.