public inbox for fortran@gcc.gnu.org
 help / color / mirror / Atom feed
* Re: [PATCH] [gfortran] Add support for allocate clause (OpenMP 5.0).
       [not found] <20211022130502.2211568-1-abidh@codesourcery.com>
@ 2021-10-22 13:28 ` Tobias Burnus
       [not found] ` <20211102162714.GF304296@tucnak>
  1 sibling, 0 replies; 21+ messages in thread
From: Tobias Burnus @ 2021-10-22 13:28 UTC (permalink / raw)
  To: Hafiz Abid Qadeer, gcc-patches, fortran; +Cc: jakub

Hi all,

On 22.10.21 15:05, Hafiz Abid Qadeer wrote:
> This patch adds support for OpenMP 5.0 allocate clause for fortran. It does not
> yet support the allocator-modifier as specified in OpenMP 5.1. The allocate
> clause is already supported in C/C++.

I think the following shouldn't block the acceptance of the patch,
but I think we eventually need to handle the following as well:

type t
   integer, allocatable :: xx(:)
end type

type(t) :: tt
class(t), allocatable :: cc

allocate(t :: cc)
tt%xx = [1,2,3,4,5,6]
cc%xx = [1,2,3,4,5,6]

! ...
!$omp task firstprivate(tt, cc) allocate(h)
  ...

In my spec reading, both tt/cc itself and tt%ii and cc%ii should
use the specified allocator.

And unless I missed something (I only glanced at the patch so far),
it is not handled.

But for derived types (except for recursive allocatables, valid since 5.1),
I think it can be handled in gfc_omp_clause_copy_ctor / gfc_omp_clause_dtor,
but I have not checked whether those support it properly.

For CLASS + recursive allocatables, it requires some more changes
(which might be provided by my derived-type deep copy patch,
of which only 1/3 has been written).

Tobias

PS: Just a side note, OpenMP has the following for Fortran:

"If any operation of the base language causes a reallocation
  of a variable that is allocated with a memory allocator then
  that memory allocator will be used to deallocate the current
  memory and to allocate the new memory. For allocated
  allocatable components of such variables, the allocator that
  will be used for the deallocation and allocation is unspecified."

-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] [gfortran] Add support for allocate clause (OpenMP 5.0).
       [not found]       ` <fddcdfcf-3fab-1674-722e-2756a1d6aef8@mentor.com>
@ 2022-01-14  9:10         ` Thomas Schwinge
  2022-01-14 11:45           ` Tobias Burnus
  2022-01-21 17:15         ` Thomas Schwinge
  1 sibling, 1 reply; 21+ messages in thread
From: Thomas Schwinge @ 2022-01-14  9:10 UTC (permalink / raw)
  To: Hafiz Abid Qadeer; +Cc: Jakub Jelinek, Tobias Burnus, gcc-patches, fortran

Hi Abid!

(Remember to CC <fortran@gcc.gnu.org> for 'gcc/fortran/' etc. changes.)


On 2022-01-11T22:31:54+0000, Hafiz Abid Qadeer <abid_qadeer@mentor.com> wrote:
> --- /dev/null
> +++ b/gcc/testsuite/gfortran.dg/gomp/allocate-2.f90
> @@ -0,0 +1,45 @@
> +! { dg-do compile }
> +
> +module omp_lib_kinds
> +  use iso_c_binding, only: c_int, c_intptr_t
> +  implicit none
> +  private :: c_int, c_intptr_t
> +  integer, parameter :: omp_allocator_handle_kind = c_intptr_t
> +
> +end module
> +
> +subroutine foo(x)
> +  use omp_lib_kinds
> +  implicit none
> +  integer  :: x
> +
> +  !$omp task allocate (x) ! { dg-error "'x' specified in 'allocate' clause at .1. but not in an explicit privatization clause" }
> +  x=1
> +  !$omp end task
> +
> +  !$omp parallel allocate (x) ! { dg-error "'x' specified in 'allocate' clause at .1. but not in an explicit privatization clause" }
> +  x=2
> +  !$omp end parallel
> +
> +  !$omp parallel allocate (x) shared (x) ! { dg-error "'x' specified in 'allocate' clause at .1. but not in an explicit privatization clause" }
> +  x=3
> +  !$omp end parallel
> +
> +  !$omp parallel private (x) allocate (x) allocate (x) ! { dg-warning "'x' appears more than once in 'allocate' clauses at .1." }
> +  x=4
> +  !$omp end parallel
> +
> +  !$omp parallel private (x) allocate (x, x) ! { dg-warning "'x' appears more than once in 'allocate' clauses at .1." }
> +  x=5
> +  !$omp end parallel
> +
> +  !$omp parallel allocate (0: x) private(x) ! { dg-error "Expected integer expression of the 'omp_allocator_handle_kind' kind at .1." }

We do for x86_64 default '-m64', but for '-m32' and '-mx32' compilation,
we're not seeing this latter diagnostic:

    PASS: gfortran.dg/gomp/allocate-1.f90   -O  (test for excess errors)
    PASS: gfortran.dg/gomp/allocate-2.f90   -O   (test for errors, line 16)
    PASS: gfortran.dg/gomp/allocate-2.f90   -O   (test for errors, line 20)
    PASS: gfortran.dg/gomp/allocate-2.f90   -O   (test for errors, line 24)
    FAIL: gfortran.dg/gomp/allocate-2.f90   -O   (test for errors, line 36)
    PASS: gfortran.dg/gomp/allocate-2.f90   -O   (test for errors, line 40)
    PASS: gfortran.dg/gomp/allocate-2.f90   -O   (test for warnings, line 28)
    PASS: gfortran.dg/gomp/allocate-2.f90   -O   (test for warnings, line 32)
    PASS: gfortran.dg/gomp/allocate-2.f90   -O  (test for excess errors)

I suppose the reason is unintended congruence of data types?  Would it
work to make 'x' a floating-point data type, for example -- or is this
meant to explicitly check certain integer data type characteristics?


Grüße
 Thomas


> +  x=6
> +  !$omp end parallel
> +
> +  !$omp parallel private (x) allocate (0.1 : x) ! { dg-error "Expected integer expression of the 'omp_allocator_handle_kind' kind at .1." }
> +  x=7
> +  !$omp end parallel
> +
> +end subroutine
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] [gfortran] Add support for allocate clause (OpenMP 5.0).
  2022-01-14  9:10         ` Thomas Schwinge
@ 2022-01-14 11:45           ` Tobias Burnus
  2022-01-14 11:55             ` Jakub Jelinek
  0 siblings, 1 reply; 21+ messages in thread
From: Tobias Burnus @ 2022-01-14 11:45 UTC (permalink / raw)
  To: Thomas Schwinge, Hafiz Abid Qadeer
  Cc: Jakub Jelinek, Tobias Burnus, gcc-patches, fortran

Hi all,

On 14.01.22 10:10, Thomas Schwinge wrote:
>> +  integer  :: x
>> ...
>> +  !$omp parallel allocate (0: x) private(x) ! { dg-error "Expected integer expression of the 'omp_allocator_handle_kind' kind at .1." }
> We do for x86_64 default '-m64', but for '-m32' and '-mx32' compilation,
> we're not seeing this latter diagnostic:
>      FAIL: gfortran.dg/gomp/allocate-2.f90   -O   (test for errors, line 36)
>
> I suppose the reason is unintended congruence of data types?  Would it
> work to make 'x' a floating-point data type, for example -- or is this
> meant to explicitly check certain integer data type characteristics?

Alternatively, you could use 'integer(kind=1)' (which is a 1-byte/8-bits
type.) I assume we do not have any platform which still uses 8-bit
pointers and supports libgomp :-)

Tobias

-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] [gfortran] Add support for allocate clause (OpenMP 5.0).
  2022-01-14 11:45           ` Tobias Burnus
@ 2022-01-14 11:55             ` Jakub Jelinek
  2022-01-14 12:20               ` Tobias Burnus
  0 siblings, 1 reply; 21+ messages in thread
From: Jakub Jelinek @ 2022-01-14 11:55 UTC (permalink / raw)
  To: Tobias Burnus; +Cc: Thomas Schwinge, Hafiz Abid Qadeer, gcc-patches, fortran

On Fri, Jan 14, 2022 at 12:45:54PM +0100, Tobias Burnus wrote:
> On 14.01.22 10:10, Thomas Schwinge wrote:
> > > +  integer  :: x
> > > ...
> > > +  !$omp parallel allocate (0: x) private(x) ! { dg-error "Expected integer expression of the 'omp_allocator_handle_kind' kind at .1." }
> > We do for x86_64 default '-m64', but for '-m32' and '-mx32' compilation,
> > we're not seeing this latter diagnostic:
> >      FAIL: gfortran.dg/gomp/allocate-2.f90   -O   (test for errors, line 36)
> > 
> > I suppose the reason is unintended congruence of data types?  Would it
> > work to make 'x' a floating-point data type, for example -- or is this
> > meant to explicitly check certain integer data type characteristics?
> 
> Alternatively, you could use 'integer(kind=1)' (which is a 1-byte/8-bits
> type.) I assume we do not have any platform which still uses 8-bit
> pointers and supports libgomp :-)

If we want to check intptr_t, we should guard the dg-error with
"" { target { lp64 || llp64 } }
or so.
Otherwise yes, we can add some other kind and hope it is not the
same as omp_allocator_handle_kind.  Or we can do both,
keep the current one with the target lp64 || llp64 and
add another one with some integer(kind=1).

	Jakub


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] [gfortran] Add support for allocate clause (OpenMP 5.0).
  2022-01-14 11:55             ` Jakub Jelinek
@ 2022-01-14 12:20               ` Tobias Burnus
  2022-01-17 14:01                 ` Hafiz Abid Qadeer
  0 siblings, 1 reply; 21+ messages in thread
From: Tobias Burnus @ 2022-01-14 12:20 UTC (permalink / raw)
  To: Jakub Jelinek, Hafiz Abid Qadeer; +Cc: gcc-patches, Thomas Schwinge, fortran

On 14.01.22 12:55, Jakub Jelinek via Fortran wrote:
> If we want to check intptr_t, we should guard the dg-error with
> "" { target { lp64 || llp64 } }
> or so.

Well, if we want to use intptr_t, we could use be explicitly as with:

   use iso_c_binding, only: c_intptr_t
   ! use omp_lib, only: omp_allocator_handle_kind
   ...  ('implicit none' etc.)
   integer, parameter :: omp_allocator_handle_kind = c_intptr_t
   ...
   integer(kind=omp_allocator_handle_kind)

(@Abid: The 'use omp_lib' line is commented as in gcc/testsuite/*/gomp/,
the OpenMP module/header from libgomp is not available - and then a
stub parameter is created.)

> Otherwise yes, we can add some other kind and hope it is not the
> same as omp_allocator_handle_kind.  Or we can do both,
> keep the current one with the target lp64 || llp64 and
> add another one with some integer(kind=1).

For just testing something invalid, I think it makes more sense to just
set it to kind=1.

For checking the valid value, using c_intptr_t seems to make more sense
than restricting it to (l)l64.

Tobias

-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] [gfortran] Add support for allocate clause (OpenMP 5.0).
  2022-01-14 12:20               ` Tobias Burnus
@ 2022-01-17 14:01                 ` Hafiz Abid Qadeer
  0 siblings, 0 replies; 21+ messages in thread
From: Hafiz Abid Qadeer @ 2022-01-17 14:01 UTC (permalink / raw)
  To: Tobias Burnus, Jakub Jelinek, Thomas Schwinge; +Cc: gcc-patches, fortran

On 14/01/2022 12:20, Tobias Burnus wrote:
> On 14.01.22 12:55, Jakub Jelinek via Fortran wrote:
>> If we want to check intptr_t, we should guard the dg-error with
>> "" { target { lp64 || llp64 } }
>> or so.
> 
> Well, if we want to use intptr_t, we could use be explicitly as with:
> 
>   use iso_c_binding, only: c_intptr_t
>   ! use omp_lib, only: omp_allocator_handle_kind
>   ...  ('implicit none' etc.)
>   integer, parameter :: omp_allocator_handle_kind = c_intptr_t
>   ...
>   integer(kind=omp_allocator_handle_kind)
> 
> (@Abid: The 'use omp_lib' line is commented as in gcc/testsuite/*/gomp/,
> the OpenMP module/header from libgomp is not available - and then a
> stub parameter is created.)
> 
>> Otherwise yes, we can add some other kind and hope it is not the
>> same as omp_allocator_handle_kind.  Or we can do both,
>> keep the current one with the target lp64 || llp64 and
>> add another one with some integer(kind=1).
> 
> For just testing something invalid, I think it makes more sense to just set it to kind=1.
> 
> For checking the valid value, using c_intptr_t seems to make more sense than restricting it to (l)l64.

Problem was with the literal 0 having same kind as c_intptr_t for m32. So there was no diagnostic in
that case. I am going to change it 0_1 to make the test more robust.

Thanks,
-- 
Hafiz Abid Qadeer
Mentor, a Siemens Business

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] [gfortran] Add support for allocate clause (OpenMP 5.0).
       [not found]       ` <fddcdfcf-3fab-1674-722e-2756a1d6aef8@mentor.com>
  2022-01-14  9:10         ` Thomas Schwinge
@ 2022-01-21 17:15         ` Thomas Schwinge
  2022-01-21 17:43           ` Tobias Burnus
  1 sibling, 1 reply; 21+ messages in thread
From: Thomas Schwinge @ 2022-01-21 17:15 UTC (permalink / raw)
  To: Hafiz Abid Qadeer; +Cc: Jakub Jelinek, Tobias Burnus, gcc-patches, fortran

Hi Abid!

On 2022-01-11T22:31:54+0000, Hafiz Abid Qadeer <abid_qadeer@mentor.com> wrote:
> From d1fb55bff497a20e6feefa50bd03890e7a903c0e Mon Sep 17 00:00:00 2001
> From: Hafiz Abid Qadeer <abidh@codesourcery.com>
> Date: Fri, 24 Sep 2021 10:04:12 +0100
> Subject: [PATCH] [gfortran] Add support for allocate clause (OpenMP 5.0).
>
> This patch adds support for OpenMP 5.0 allocate clause for fortran. It does not
> yet support the allocator-modifier as specified in OpenMP 5.1. The allocate
> clause is already supported in C/C++.

> libgomp/ChangeLog:
>
>       * testsuite/libgomp.fortran/allocate-1.c: New test.
>       * testsuite/libgomp.fortran/allocate-1.f90: New test.

I'm seeing this test case randomly/non-deterministically FAIL to execute,
differently on different systems and runs, for example:

    libgomp:
    libgomp:
    libgomp: Out of memory allocating 4 bytesOut of memory allocating 4 bytes
    libgomp:
    libgomp:
    libgomp: Out of memory allocating 168 bytes

    libgomp: Out of memory allocating 4 bytes

    libgomp: Out of memory allocating 4 bytes

    libgomp: Out of memory allocating 4 bytes

I'd assume there's some concurrency issue: the problem disappears if I
manually specify a lowerish 'OMP_NUM_THREADS', and conversely, on a
system where I don't normally see the FAILs, I can trigger them with a
largish 'OMP_NUM_THREADS', such as 'OMP_NUM_THREADS=18' and higher.

For example:

    Thread 10 "a.out" hit Breakpoint 1, omp_aligned_alloc (alignment=4, size=4, allocator=6326576) at [...]/source-gcc/libgomp/allocator.c:318
    318       if (allocator_data)
    (gdb) print *allocator_data
    $1 = {memspace = omp_default_mem_space, alignment = 64, pool_size = 8192, used_pool_size = 8188, fb_data = omp_null_allocator, sync_hint = 3, access = 7, fallback = 12, pinned = 0, partition = 15}

Given the high 'used_pool_size', is that to be expected, and the test
case shouldn't be requesting "so much" memory?  Or might the problem
actually be in 'libgomp/allocator.c' (not touched by your commit)?

All but Thread 10 are in 'gomp_team_barrier_wait_end' -- should memory
have been released at that point?

    (gdb) thread apply 10 bt

    Thread 10 (Thread 0x7ffff32e2700 (LWP 1601318)):
    #0  omp_aligned_alloc (alignment=4, size=4, allocator=6326576) at [...]/source-gcc/libgomp/allocator.c:320
    #1  0x00007ffff790b4db in GOMP_alloc (alignment=4, size=4, allocator=6326576) at [...]/source-gcc/libgomp/allocator.c:364
    #2  0x0000000000401f3f in foo_._omp_fn.3 () at source-gcc/libgomp/testsuite/libgomp.fortran/allocate-1.f90:136
    #3  0x00007ffff78f31e6 in gomp_thread_start (xdata=<optimized out>) at [...]/source-gcc/libgomp/team.c:129
    #4  0x00007ffff789e609 in start_thread (arg=<optimized out>) at pthread_create.c:477
    #5  0x00007ffff77c5293 in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
    (gdb) thread apply 1 bt

    Thread 1 (Thread 0x7ffff72ec1c0 (LWP 1601309)):
    #0  futex_wait (val=96, addr=<optimized out>) at [...]/source-gcc/libgomp/config/linux/x86/futex.h:97
    #1  do_wait (val=96, addr=<optimized out>) at [...]/source-gcc/libgomp/config/linux/wait.h:67
    #2  gomp_team_barrier_wait_end (bar=<optimized out>, state=96) at [...]/source-gcc/libgomp/config/linux/bar.c:112
    #3  0x0000000000401f53 in foo_._omp_fn.3 () at source-gcc/libgomp/testsuite/libgomp.fortran/allocate-1.f90:136
    #4  0x00007ffff78ea4f2 in GOMP_parallel (fn=0x401e6b <foo_._omp_fn.3>, data=0x7fffffffd450, num_threads=18, flags=0) at [...]/source-gcc/libgomp/parallel.c:178
    #5  0x00000000004012ab in foo (x=42, p=..., q=..., px=2, h=6326576, fl=0) at source-gcc/libgomp/testsuite/libgomp.fortran/allocate-1.f90:122
    #6  0x00000000004018e9 in MAIN__ () at source-gcc/libgomp/testsuite/libgomp.fortran/allocate-1.f90:326

Manually compiling the test case, I see a lot of '-Wtabs' diagnostics
(can be ignored, I suppose), but also:

    source-gcc/libgomp/testsuite/libgomp.fortran/allocate-1.f90:11:47:

       11 |     integer(c_int) function is_64bit_aligned (a) bind(C)
          |                                               1
    Warning: Variable ‘a’ at (1) is a dummy argument of the BIND(C) procedure ‘is_64bit_aligned’ but may not be C interoperable [-Wc-binding-type]

Is that something to worry about?

And:

    source-gcc/libgomp/testsuite/libgomp.fortran/allocate-1.f90:31:19:

       31 |   integer  :: n, n1, n2, n3, n4
          |                   1
    Warning: Unused variable ‘n1’ declared at (1) [-Wunused-variable]
    source-gcc/libgomp/testsuite/libgomp.fortran/allocate-1.f90:18:27:

       18 | subroutine foo (x, p, q, px, h, fl)
          |                           1
    Warning: Unused dummy argument ‘px’ at (1) [-Wunused-dummy-argument]

For reference, quoting below the new Fortran test case.


Grüße
 Thomas


> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.fortran/allocate-1.c
> @@ -0,0 +1,7 @@
> +#include <stdint.h>
> +
> +int
> +is_64bit_aligned_ (uintptr_t a)
> +{
> +  return ( (a & 0x3f) == 0);
> +}

> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.fortran/allocate-1.f90
> @@ -0,0 +1,333 @@
> +! { dg-do run }
> +! { dg-additional-sources allocate-1.c }
> +! { dg-prune-output "command-line option '-fintrinsic-modules-path=.*' is valid for Fortran but not for C" }
> +
> +module m
> +  use omp_lib
> +  use iso_c_binding
> +  implicit none
> +
> +  interface
> +    integer(c_int) function is_64bit_aligned (a) bind(C)
> +      import :: c_int
> +      integer  :: a
> +    end
> +  end interface
> +end module m
> +
> +subroutine foo (x, p, q, px, h, fl)
> +  use omp_lib
> +  use iso_c_binding
> +  integer  :: x
> +  integer, dimension(4) :: p
> +  integer, dimension(4) :: q
> +  integer  :: px
> +  integer (kind=omp_allocator_handle_kind) :: h
> +  integer  :: fl
> +
> +  integer  :: y
> +  integer  :: r, i, i1, i2, i3, i4, i5
> +  integer  :: l, l3, l4, l5, l6
> +  integer  :: n, n1, n2, n3, n4
> +  integer  :: j2, j3, j4
> +  integer, dimension(4) :: l2
> +  integer, dimension(4) :: r2
> +  integer, target  :: xo
> +  integer, target  :: yo
> +  integer, dimension(x) :: v
> +  integer, dimension(x) :: w
> +
> +  type s_type
> +    integer      :: a
> +    integer      :: b
> +  end type
> +
> +  type (s_type) :: s
> +  s%a = 27
> +  s%b = 29
> +  y = 0
> +  r = 0
> +  n = 8
> +  n2 = 9
> +  n3 = 10
> +  n4 = 11
> +  xo = x
> +  yo = y
> +
> +  do i = 1, 4
> +    r2(i) = 0;
> +  end do
> +
> +  do i = 1, 4
> +    p(i) = 0;
> +  end do
> +
> +  do i = 1, 4
> +    q(i) = 0;
> +  end do
> +
> +  do i = 1, x
> +    w(i) = i
> +  end do
> +
> +  !$omp parallel private (y, v) firstprivate (x) allocate (x, y, v)
> +  if (x /= 42) then
> +    stop 1
> +  end if
> +  v(1) = 7
> +  if ( (and(fl, 2) /= 0) .and.          &
> +       ((is_64bit_aligned(x) == 0) .or. &
> +        (is_64bit_aligned(y) == 0) .or. &
> +        (is_64bit_aligned(v(1)) == 0))) then
> +      stop 2
> +  end if
> +
> +  !$omp barrier
> +  y = 1;
> +  x = x + 1
> +  v(1) = 7
> +  v(41) = 8
> +  !$omp barrier
> +  if (x /= 43 .or. y /= 1) then
> +    stop 3
> +  end if
> +  if (v(1) /= 7 .or. v(41) /= 8) then
> +    stop 4
> +  end if
> +  !$omp end parallel
> +
> +  !$omp teams
> +  !$omp parallel private (y) firstprivate (x, w) allocate (h: x, y, w)
> +
> +  if (x /= 42 .or. w(17) /= 17 .or. w(41) /= 41) then
> +    stop 5
> +  end if
> +  !$omp barrier
> +  y = 1;
> +  x = x + 1
> +  w(19) = w(19) + 1
> +  !$omp barrier
> +  if (x /= 43 .or. y /= 1 .or. w(19) /= 20) then
> +    stop 6
> +  end if
> +  if ( (and(fl, 1) /= 0) .and.          &
> +       ((is_64bit_aligned(x) == 0) .or. &
> +        (is_64bit_aligned(y) == 0) .or. &
> +        (is_64bit_aligned(w(1)) == 0))) then
> +    stop 7
> +  end if
> +  !$omp end parallel
> +  !$omp end teams
> +
> +  !$omp parallel do private (y) firstprivate (x)  reduction(+: r) allocate (h: x, y, r, l, n) lastprivate (l)  linear (n: 16)
> +  do i = 0, 63
> +    if (x /= 42) then
> +      stop 8
> +    end if
> +    y = 1;
> +    l = i;
> +    n = n + y + 15;
> +    r = r + i;
> +    if ( (and(fl, 1) /= 0) .and.          &
> +         ((is_64bit_aligned(x) == 0) .or. &
> +          (is_64bit_aligned(y) == 0) .or. &
> +          (is_64bit_aligned(r) == 0) .or. &
> +          (is_64bit_aligned(l) == 0) .or. &
> +          (is_64bit_aligned(n) == 0))) then
> +      stop 9
> +    end if
> +  end do
> +  !$omp end parallel do
> +
> +  !$omp parallel
> +    !$omp do lastprivate (l2) private (i1) allocate (h: l2, l3, i1) lastprivate (conditional: l3)
> +    do i1 = 0, 63
> +      l2(1) = i1
> +      l2(2) = i1 + 1
> +      l2(3) = i1 + 2
> +      l2(4) = i1 + 3
> +      if (i1 < 37) then
> +        l3 = i1
> +      end if
> +      if ( (and(fl, 1) /= 0) .and.          &
> +           ((is_64bit_aligned(l2(1)) == 0) .or. &
> +            (is_64bit_aligned(l3) == 0) .or. &
> +            (is_64bit_aligned(i1) == 0))) then
> +     stop 10
> +      end if
> +    end do
> +
> +    !$omp do collapse(2) lastprivate(l4, i2, j2) linear (n2:17) allocate (h: n2, l4, i2, j2)
> +    do i2 = 3, 4
> +      do j2 = 17, 22, 2
> +     n2 = n2 + 17
> +     l4 = i2 * 31 + j2
> +     if ( (and(fl, 1) /= 0) .and.          &
> +       ((is_64bit_aligned(l4) == 0) .or. &
> +       (is_64bit_aligned(n2) == 0) .or. &
> +       (is_64bit_aligned(i2) == 0) .or. &
> +       (is_64bit_aligned(j2) == 0))) then
> +       stop 11
> +     end if
> +      end do
> +    end do
> +
> +    !$omp do collapse(2) lastprivate(l5, i3, j3) linear (n3:17) schedule (static, 3) allocate (n3, l5, i3, j3)
> +    do i3 = 3, 4
> +      do j3 = 17, 22, 2
> +       n3 = n3 + 17
> +       l5 = i3 * 31 + j3
> +       if ( (and(fl, 2) /= 0) .and.      &
> +       ((is_64bit_aligned(l5) == 0) .or. &
> +       (is_64bit_aligned(n3) == 0) .or. &
> +       (is_64bit_aligned(i3) == 0) .or. &
> +       (is_64bit_aligned(j3) == 0))) then
> +       stop 12
> +     end if
> +      end do
> +    end do
> +
> +    !$omp do collapse(2) lastprivate(l6, i4, j4) linear (n4:17) schedule (dynamic) allocate (h: n4, l6, i4, j4)
> +    do i4 = 3, 4
> +      do j4 = 17, 22,2
> +       n4 = n4 + 17;
> +       l6 = i4 * 31 + j4;
> +     if ( (and(fl, 1) /= 0) .and.          &
> +       ((is_64bit_aligned(l6) == 0) .or. &
> +       (is_64bit_aligned(n4) == 0) .or. &
> +       (is_64bit_aligned(i4) == 0) .or. &
> +       (is_64bit_aligned(j4) == 0))) then
> +       stop 13
> +     end if
> +      end do
> +    end do
> +
> +    !$omp do lastprivate (i5) allocate (i5)
> +    do i5 = 1, 17, 3
> +      if ( (and(fl, 2) /= 0) .and.          &
> +        (is_64bit_aligned(i5) == 0)) then
> +     stop 14
> +      end if
> +    end do
> +
> +    !$omp do reduction(+:p, q, r2) allocate(h: p, q, r2)
> +    do i = 0, 31
> +     p(3) = p(3) +  i;
> +     p(4) = p(4) + (2 * i)
> +     q(1) = q(1) + (3 * i)
> +     q(3) = q(3) + (4 * i)
> +     r2(1) = r2(1) + (5 * i)
> +     r2(4) = r2(4) + (6 * i)
> +     if ( (and(fl, 1) /= 0) .and.          &
> +       ((is_64bit_aligned(q(1)) == 0) .or. &
> +       (is_64bit_aligned(p(1)) == 0) .or. &
> +       (is_64bit_aligned(r2(1)) == 0) )) then
> +       stop 15
> +     end if
> +    end do
> +
> +    !$omp task private(y) firstprivate(x) allocate(x, y)
> +    if (x /= 42) then
> +      stop 16
> +    end if
> +
> +    if ( (and(fl, 2) /= 0) .and.          &
> +      ((is_64bit_aligned(x) == 0) .or. &
> +      (is_64bit_aligned(y) == 0) )) then
> +      stop 17
> +    end if
> +    !$omp end task
> +
> +    !$omp task private(y) firstprivate(x) allocate(h: x, y)
> +    if (x /= 42) then
> +      stop 16
> +    end if
> +
> +    if ( (and(fl, 1) /= 0) .and.          &
> +      ((is_64bit_aligned(x) == 0) .or. &
> +      (is_64bit_aligned(y) == 0) )) then
> +      stop 17
> +    end if
> +    !$omp end task
> +
> +    !$omp task private(y) firstprivate(s) allocate(s, y)
> +    if (s%a /= 27 .or. s%b /= 29) then
> +      stop 18
> +    end if
> +
> +    if ( (and(fl, 2) /= 0) .and.          &
> +      ((is_64bit_aligned(s%a) == 0) .or. &
> +      (is_64bit_aligned(y) == 0) )) then
> +      stop 19
> +    end if
> +    !$omp end task
> +
> +    !$omp task private(y) firstprivate(s) allocate(h: s, y)
> +    if (s%a /= 27 .or. s%b /= 29) then
> +      stop 18
> +    end if
> +
> +    if ( (and(fl, 1) /= 0) .and.          &
> +      ((is_64bit_aligned(s%a) == 0) .or. &
> +      (is_64bit_aligned(y) == 0) )) then
> +      stop 19
> +    end if
> +    !$omp end task
> +
> +  !$omp end parallel
> +
> +  if (r /= ((64 * 63) / 2) .or. l /= 63 .or. n /= (8 + 16 * 64)) then
> +    stop 20
> +  end if
> +
> +  if (l2(1) /= 63 .or. l2(2) /= 64 .or. l2(3) /= 65 .or. l2(4) /= 66 .or. l3 /= 36) then
> +    stop 21
> +  end if
> +
> +  if (i2 /= 5 .or. j2 /= 23 .or. n2 /= (9 + (17 * 6)) .or. l4 /= (4 * 31 + 21)) then
> +    stop 22
> +  end if
> +
> +  if (i3 /= 5 .or. j3 /= 23 .or. n3 /= (10 + (17 * 6))  .or. l5 /= (4 * 31 + 21)) then
> +    stop 23
> +  end if
> +
> +  if (i4 /= 5 .or. j4 /= 23 .or. n4 /= (11 + (17 * 6))  .or. l6 /= (4 * 31 + 21)) then
> +    stop 24
> +  end if
> +
> +  if (i5 /= 19) then
> +    stop 24
> +  end if
> +
> +  if (p(3) /= ((32 * 31) / 2) .or. p(4) /= (2 * p(3))         &
> +      .or. q(1) /= (3 * p(3)) .or. q(3) /= (4 * p(3))         &
> +      .or. r2(1) /= (5 * p(3)) .or. r2(4) /= (6 * p(3))) then
> +    stop 25
> +  end if
> +
> +end subroutine
> +
> +program main
> +  use omp_lib
> +  integer, dimension(4) :: p
> +  integer, dimension(4) :: q
> +
> +  type (omp_alloctrait) :: traits(3)
> +  integer (omp_allocator_handle_kind) :: a
> +
> +  traits = [omp_alloctrait (omp_atk_alignment, 64), &
> +            omp_alloctrait (omp_atk_fallback, omp_atv_null_fb), &
> +            omp_alloctrait (omp_atk_pool_size, 8192)]
> +  a = omp_init_allocator (omp_default_mem_space, 3, traits)
> +  if (a == omp_null_allocator) stop 1
> +
> +  call omp_set_default_allocator (omp_default_mem_alloc);
> +  call foo (42, p, q, 2, a, 0);
> +  call foo (42, p, q, 2, omp_default_mem_alloc, 0);
> +  call foo (42, p, q, 2, a, 1);
> +  call omp_set_default_allocator (a);
> +  call foo (42, p, q, 2, omp_null_allocator, 3);
> +  call foo (42, p, q, 2, omp_default_mem_alloc, 2);
> +  call omp_destroy_allocator (a);
> +end
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] [gfortran] Add support for allocate clause (OpenMP 5.0).
  2022-01-21 17:15         ` Thomas Schwinge
@ 2022-01-21 17:43           ` Tobias Burnus
  2022-01-24  8:45             ` Tobias Burnus
  0 siblings, 1 reply; 21+ messages in thread
From: Tobias Burnus @ 2022-01-21 17:43 UTC (permalink / raw)
  To: Thomas Schwinge, Hafiz Abid Qadeer
  Cc: Jakub Jelinek, Tobias Burnus, gcc-patches, fortran

On 21.01.22 18:15, Thomas Schwinge wrote:
>      source-gcc/libgomp/testsuite/libgomp.fortran/allocate-1.f90:11:47:
>
>         11 |     integer(c_int) function is_64bit_aligned (a) bind(C)
>            |                                               1
>      Warning: Variable ‘a’ at (1) is a dummy argument of the BIND(C) procedure ‘is_64bit_aligned’ but may not be C interoperable [-Wc-binding-type]
>
> Is that something to worry about?

I think it is not very elegant – but should be okay.

On the Fortran side:

     integer(c_int) function is_64bit_aligned (a) bind(C)
       import :: c_int
       integer  :: a
     end

that matches  'int is_64bit_aligned (int *a);'
While 'integer' in principle may not be 'int',
the call by reference makes this independent of the
actually used integer kind.

HOWEVER: That interface it not used! While it
defines that interface in 'module m', there is
no 'use m' in 'subroutine foo'.

(or alternatively: 'foo' being after 'contains' inside
the 'module m' - and then 'use m' in the main program)



That means that 'is_64bit_aligned(...)' gets implicitly
types as 'integer' with unknown arguments, which get
passed by value. By gfortran convention, that function
has a tailing underscore.

That matches the C side, which such an underscore:

int
is_64bit_aligned_ (uintptr_t a)
{
   return ( (a & 0x3f) == 0);
}

With pass by reference, a pointer is passed, which
should be handled by 'uintptr_t'.

  * * *

Side remark: I really recommend 'implicit none'
when writing Fortran code - which disables implicit
typing. I personally have started to use
   implicit none (type, external)
which also rejects 'call something()' unless
'something' has been explicitly declared, e.g. by
an interface block.

  * * *

Side remark: A Fortran-only variant has been used in
libgomp/testsuite/libgomp.fortran/alloc-11.f90:

if (mod (TRANSFER (p, iptr), 64) /= 0)

As optimization, also 'iand(..., z'3f') == 0' would work ;-)

Tobias

-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] [gfortran] Add support for allocate clause (OpenMP 5.0).
  2022-01-21 17:43           ` Tobias Burnus
@ 2022-01-24  8:45             ` Tobias Burnus
  2022-01-24 12:54               ` Hafiz Abid Qadeer
  2022-02-04  9:37               ` Thomas Schwinge
  0 siblings, 2 replies; 21+ messages in thread
From: Tobias Burnus @ 2022-01-24  8:45 UTC (permalink / raw)
  To: Tobias Burnus, Thomas Schwinge, Hafiz Abid Qadeer
  Cc: Jakub Jelinek, gcc-patches, fortran

[-- Attachment #1: Type: text/plain, Size: 2676 bytes --]

On 21.01.22 18:43, Tobias Burnus wrote:
> On 21.01.22 18:15, Thomas Schwinge wrote:
>>         11 |     integer(c_int) function is_64bit_aligned (a) bind(C)
>>      Warning: Variable ‘a’ at (1) is a dummy argument of the BIND(C)
>> procedure ‘is_64bit_aligned’ but may not be C interoperable
>> [-Wc-binding-type]
>>
>> Is that something to worry about?
I have attached a patch (not commited), which silences the three kind of
warnings and fixes the interface issue.
TODO: commit it.

On 21.01.22 18:15, Thomas Schwinge wrote:
> I'm seeing this test case randomly/non-deterministically FAIL to execute,
> differently on different systems and runs, for example: [...]
> I'd assume there's some concurrency issue: the problem disappears if I
> manually specify a lowerish 'OMP_NUM_THREADS'

If one compiles the program with -fsanitize=thread, it shows tons of errors :-(
The first one is:

WARNING: ThreadSanitizer: data race (pid=3034413)
   Read of size 8 at 0x7fff8b5a8340 by thread T1:
     #0 __m_MOD_foo._omp_fn.2 ../../libgomp/testsuite/libgomp.fortran/allocate-1.f90:116 (a.out+0x402a88)
     #1 gomp_thread_start ../../../repos/gcc-trunk-commit/libgomp/team.c:129 (libgomp.so.1+0x1e5ed)

   Previous write of size 8 at 0x7fff8b5a8340 by main thread:
     #0 __m_MOD_foo._omp_fn.1 ../../libgomp/testsuite/libgomp.fortran/allocate-1.f90:116 (a.out+0x4029c0)
     #1 GOMP_teams_reg ../../../repos/gcc-trunk-commit/libgomp/teams.c:51 (libgomp.so.1+0x3638c)
     #2 MAIN__ ../../libgomp/testsuite/libgomp.fortran/allocate-1.f90:328 (a.out+0x4024c0)
     #3 main ../../libgomp/testsuite/libgomp.fortran/allocate-1.f90:312 (a.out+0x4025b0)

   Location is stack of main thread.

   Location is global '<null>' at 0x000000000000 ([stack]+0x1f340)

   Thread T1 (tid=3034416, running) created by main thread at:
     #0 pthread_create ../../../../repos/gcc-trunk-commit/libsanitizer/tsan/tsan_interceptors_posix.cpp:1001 (libtsan.so.2+0x62c76)
     #1 gomp_team_start ../../../repos/gcc-trunk-commit/libgomp/team.c:858 (libgomp.so.1+0x1ec18)
     #2 MAIN__ ../../libgomp/testsuite/libgomp.fortran/allocate-1.f90:328 (a.out+0x4024c0)
     #3 main ../../libgomp/testsuite/libgomp.fortran/allocate-1.f90:312 (a.out+0x4025b0)

SUMMARY: ThreadSanitizer: data race ../../libgomp/testsuite/libgomp.fortran/allocate-1.f90:116 in __m_MOD_foo._omp_fn.2

Tobias
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 6422 bytes --]

diff --git a/libgomp/testsuite/libgomp.fortran/allocate-1.c b/libgomp/testsuite/libgomp.fortran/allocate-1.c
index d33acc6feef..cb6d355afc6 100644
--- a/libgomp/testsuite/libgomp.fortran/allocate-1.c
+++ b/libgomp/testsuite/libgomp.fortran/allocate-1.c
@@ -1,7 +1,7 @@
 #include <stdint.h>
 
 int
-is_64bit_aligned_ (uintptr_t a)
+is_64bit_aligned (uintptr_t a)
 {
   return ( (a & 0x3f) == 0);
 }
diff --git a/libgomp/testsuite/libgomp.fortran/allocate-1.f90 b/libgomp/testsuite/libgomp.fortran/allocate-1.f90
index 35d1750b878..f31bd533e7f 100644
--- a/libgomp/testsuite/libgomp.fortran/allocate-1.f90
+++ b/libgomp/testsuite/libgomp.fortran/allocate-1.f90
@@ -5,30 +5,30 @@
 module m
   use omp_lib
   use iso_c_binding
-  implicit none
+  implicit none (type, external)
 
   interface
     integer(c_int) function is_64bit_aligned (a) bind(C)
       import :: c_int
-      integer  :: a
+      type(*)  :: a
     end
   end interface
-end module m
 
-subroutine foo (x, p, q, px, h, fl)
+contains
+
+subroutine foo (x, p, q, h, fl)
   use omp_lib
   use iso_c_binding
   integer  :: x
   integer, dimension(4) :: p
   integer, dimension(4) :: q
-  integer  :: px
   integer (kind=omp_allocator_handle_kind) :: h
   integer  :: fl
 
   integer  :: y
   integer  :: r, i, i1, i2, i3, i4, i5
   integer  :: l, l3, l4, l5, l6
-  integer  :: n, n1, n2, n3, n4
+  integer  :: n, n2, n3, n4
   integer  :: j2, j3, j4
   integer, dimension(4) :: l2
   integer, dimension(4) :: r2
@@ -118,6 +118,7 @@ subroutine foo (x, p, q, px, h, fl)
   end if
   !$omp end parallel
   !$omp end teams
+stop
 
   !$omp parallel do private (y) firstprivate (x)  reduction(+: r) allocate (h: x, y, r, l, n) lastprivate (l)  linear (n: 16)
   do i = 0, 63
@@ -153,77 +154,77 @@ subroutine foo (x, p, q, px, h, fl)
            ((is_64bit_aligned(l2(1)) == 0) .or. &
             (is_64bit_aligned(l3) == 0) .or. &
             (is_64bit_aligned(i1) == 0))) then
-	stop 10
+        stop 10
       end if
     end do
 
     !$omp do collapse(2) lastprivate(l4, i2, j2) linear (n2:17) allocate (h: n2, l4, i2, j2)
     do i2 = 3, 4
       do j2 = 17, 22, 2
-	n2 = n2 + 17
-	l4 = i2 * 31 + j2
-	if ( (and(fl, 1) /= 0) .and.          &
-	  ((is_64bit_aligned(l4) == 0) .or. &
-	  (is_64bit_aligned(n2) == 0) .or. &
-	  (is_64bit_aligned(i2) == 0) .or. &
-	  (is_64bit_aligned(j2) == 0))) then
-	  stop 11
-	end if
+        n2 = n2 + 17
+        l4 = i2 * 31 + j2
+        if ( (and(fl, 1) /= 0) .and.          &
+             ((is_64bit_aligned(l4) == 0) .or. &
+              (is_64bit_aligned(n2) == 0) .or. &
+              (is_64bit_aligned(i2) == 0) .or. &
+              (is_64bit_aligned(j2) == 0))) then
+          stop 11
+        end if
       end do
     end do
 
     !$omp do collapse(2) lastprivate(l5, i3, j3) linear (n3:17) schedule (static, 3) allocate (n3, l5, i3, j3)
     do i3 = 3, 4
       do j3 = 17, 22, 2
-	  n3 = n3 + 17
-	  l5 = i3 * 31 + j3
-	  if ( (and(fl, 2) /= 0) .and.      &
-	  ((is_64bit_aligned(l5) == 0) .or. &
-	  (is_64bit_aligned(n3) == 0) .or. &
-	  (is_64bit_aligned(i3) == 0) .or. &
-	  (is_64bit_aligned(j3) == 0))) then
-	  stop 12
-	end if
+          n3 = n3 + 17
+          l5 = i3 * 31 + j3
+          if ( (and(fl, 2) /= 0) .and.      &
+             ((is_64bit_aligned(l5) == 0) .or. &
+              (is_64bit_aligned(n3) == 0) .or. &
+              (is_64bit_aligned(i3) == 0) .or. &
+              (is_64bit_aligned(j3) == 0))) then
+          stop 12
+        end if
       end do
     end do
 
     !$omp do collapse(2) lastprivate(l6, i4, j4) linear (n4:17) schedule (dynamic) allocate (h: n4, l6, i4, j4)
     do i4 = 3, 4
       do j4 = 17, 22,2
-	  n4 = n4 + 17;
-	  l6 = i4 * 31 + j4;
-	if ( (and(fl, 1) /= 0) .and.          &
-	  ((is_64bit_aligned(l6) == 0) .or. &
-	  (is_64bit_aligned(n4) == 0) .or. &
-	  (is_64bit_aligned(i4) == 0) .or. &
-	  (is_64bit_aligned(j4) == 0))) then
-	  stop 13
-	end if
+          n4 = n4 + 17;
+          l6 = i4 * 31 + j4;
+        if ( (and(fl, 1) /= 0) .and.          &
+            ((is_64bit_aligned(l6) == 0) .or. &
+             (is_64bit_aligned(n4) == 0) .or. &
+             (is_64bit_aligned(i4) == 0) .or. &
+             (is_64bit_aligned(j4) == 0))) then
+          stop 13
+        end if
       end do
     end do
 
     !$omp do lastprivate (i5) allocate (i5)
     do i5 = 1, 17, 3
       if ( (and(fl, 2) /= 0) .and.          &
-	   (is_64bit_aligned(i5) == 0)) then
-	stop 14
+           (is_64bit_aligned(i5) == 0)) then
+        stop 14
       end if
     end do
 
     !$omp do reduction(+:p, q, r2) allocate(h: p, q, r2)
     do i = 0, 31
-	p(3) = p(3) +  i;
-	p(4) = p(4) + (2 * i)
-	q(1) = q(1) + (3 * i)
-	q(3) = q(3) + (4 * i)
-	r2(1) = r2(1) + (5 * i)
-	r2(4) = r2(4) + (6 * i)
-	if ( (and(fl, 1) /= 0) .and.          &
-	  ((is_64bit_aligned(q(1)) == 0) .or. &
-	  (is_64bit_aligned(p(1)) == 0) .or. &
-	  (is_64bit_aligned(r2(1)) == 0) )) then
-	  stop 15
-	end if
+        p(3) = p(3) +  i;
+        p(4) = p(4) + (2 * i)
+        q(1) = q(1) + (3 * i)
+        q(3) = q(3) + (4 * i)
+        r2(1) = r2(1) + (5 * i)
+        r2(4) = r2(4) + (6 * i)
+        if ( (and(fl, 1) /= 0) .and.             &
+             ((is_64bit_aligned(q(1)) == 0) .or. &
+              (is_64bit_aligned(p(1)) == 0) .or. &
+              (is_64bit_aligned(r2(1)) == 0) )) then
+          stop 15
+        end if
     end do
 
     !$omp task private(y) firstprivate(x) allocate(x, y)
@@ -305,11 +306,13 @@ subroutine foo (x, p, q, px, h, fl)
       .or. r2(1) /= (5 * p(3)) .or. r2(4) /= (6 * p(3))) then
     stop 25
   end if
-
 end subroutine
+end module m
 
 program main
   use omp_lib
+  use m
+  implicit none (type, external)
   integer, dimension(4) :: p
   integer, dimension(4) :: q
 
@@ -323,11 +326,11 @@ program main
   if (a == omp_null_allocator) stop 1
 
   call omp_set_default_allocator (omp_default_mem_alloc);
-  call foo (42, p, q, 2, a, 0);
-  call foo (42, p, q, 2, omp_default_mem_alloc, 0);
-  call foo (42, p, q, 2, a, 1);
+  call foo (42, p, q, a, 0);
+  call foo (42, p, q, omp_default_mem_alloc, 0);
+  call foo (42, p, q, a, 1);
   call omp_set_default_allocator (a);
-  call foo (42, p, q, 2, omp_null_allocator, 3);
-  call foo (42, p, q, 2, omp_default_mem_alloc, 2);
+  call foo (42, p, q, omp_null_allocator, 3);
+  call foo (42, p, q, omp_default_mem_alloc, 2);
   call omp_destroy_allocator (a);
 end

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] [gfortran] Add support for allocate clause (OpenMP 5.0).
  2022-01-24  8:45             ` Tobias Burnus
@ 2022-01-24 12:54               ` Hafiz Abid Qadeer
  2022-01-25  9:19                 ` Thomas Schwinge
  2022-02-04  9:37               ` Thomas Schwinge
  1 sibling, 1 reply; 21+ messages in thread
From: Hafiz Abid Qadeer @ 2022-01-24 12:54 UTC (permalink / raw)
  To: Tobias Burnus, Thomas Schwinge, Hafiz Abid Qadeer
  Cc: Jakub Jelinek, gcc-patches, fortran

On 24/01/2022 08:45, Tobias Burnus wrote:
> On 21.01.22 18:43, Tobias Burnus wrote:
>> On 21.01.22 18:15, Thomas Schwinge wrote:
>>>         11 |     integer(c_int) function is_64bit_aligned (a) bind(C)
>>>      Warning: Variable ‘a’ at (1) is a dummy argument of the BIND(C) procedure ‘is_64bit_aligned’
>>> but may not be C interoperable [-Wc-binding-type]
>>>
>>> Is that something to worry about?
> I have attached a patch (not commited), which silences the three kind of warnings and fixes the
> interface issue.
> TODO: commit it.
> 
> On 21.01.22 18:15, Thomas Schwinge wrote:
>> I'm seeing this test case randomly/non-deterministically FAIL to execute,
>> differently on different systems and runs, for example: [...]
>> I'd assume there's some concurrency issue: the problem disappears if I
>> manually specify a lowerish 'OMP_NUM_THREADS'
> 
> If one compiles the program with -fsanitize=thread, it shows tons of errors :-(
> The first one is:
> 
> WARNING: ThreadSanitizer: data race (pid=3034413)
>   Read of size 8 at 0x7fff8b5a8340 by thread T1:
>     #0 __m_MOD_foo._omp_fn.2 ../../libgomp/testsuite/libgomp.fortran/allocate-1.f90:116
> (a.out+0x402a88)
>     #1 gomp_thread_start ../../../repos/gcc-trunk-commit/libgomp/team.c:129 (libgomp.so.1+0x1e5ed)
> 
>   Previous write of size 8 at 0x7fff8b5a8340 by main thread:
>     #0 __m_MOD_foo._omp_fn.1 ../../libgomp/testsuite/libgomp.fortran/allocate-1.f90:116
> (a.out+0x4029c0)
>     #1 GOMP_teams_reg ../../../repos/gcc-trunk-commit/libgomp/teams.c:51 (libgomp.so.1+0x3638c)
>     #2 MAIN__ ../../libgomp/testsuite/libgomp.fortran/allocate-1.f90:328 (a.out+0x4024c0)
>     #3 main ../../libgomp/testsuite/libgomp.fortran/allocate-1.f90:312 (a.out+0x4025b0)
> 
>   Location is stack of main thread.
> 
>   Location is global '<null>' at 0x000000000000 ([stack]+0x1f340)
> 
>   Thread T1 (tid=3034416, running) created by main thread at:
>     #0 pthread_create
> ../../../../repos/gcc-trunk-commit/libsanitizer/tsan/tsan_interceptors_posix.cpp:1001
> (libtsan.so.2+0x62c76)
>     #1 gomp_team_start ../../../repos/gcc-trunk-commit/libgomp/team.c:858 (libgomp.so.1+0x1ec18)
>     #2 MAIN__ ../../libgomp/testsuite/libgomp.fortran/allocate-1.f90:328 (a.out+0x4024c0)
>     #3 main ../../libgomp/testsuite/libgomp.fortran/allocate-1.f90:312 (a.out+0x4025b0)
> 
> SUMMARY: ThreadSanitizer: data race ../../libgomp/testsuite/libgomp.fortran/allocate-1.f90:116 in
> __m_MOD_foo._omp_fn.2
> 
> Tobias
@Tobias: Thanks for your comments and the patch.

@Thomas: Thanks for reporting the problem. Did you notice similar behavior with
libgomp/testsuite/libgomp.c-c++-common/allocate-1.c? It was used as base for fortran testcase and it
shows similar warnings with -fthread=sanitize. I am trying to figure out if the problem you observed
is a general one or just specific to fortran testcase.

-- 
Hafiz Abid Qadeer
Mentor, a Siemens Business

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] [gfortran] Add support for allocate clause (OpenMP 5.0).
  2022-01-24 12:54               ` Hafiz Abid Qadeer
@ 2022-01-25  9:19                 ` Thomas Schwinge
  2022-01-25 10:32                   ` Tobias Burnus
  0 siblings, 1 reply; 21+ messages in thread
From: Thomas Schwinge @ 2022-01-25  9:19 UTC (permalink / raw)
  To: Jakub Jelinek, Tobias Burnus, Hafiz Abid Qadeer; +Cc: gcc-patches, fortran

Hi!

On 2022-01-24T12:54:27+0000, Hafiz Abid Qadeer <abid_qadeer@mentor.com> wrote:
> On 24/01/2022 08:45, Tobias Burnus wrote:
>> On 21.01.22 18:15, Thomas Schwinge wrote:
>>> I'm seeing this test case randomly/non-deterministically FAIL to execute,
>>> differently on different systems and runs, for example: [...]
>>> I'd assume there's some concurrency issue: the problem disappears if I
>>> manually specify a lowerish 'OMP_NUM_THREADS'
>>
>> If one compiles the program with -fsanitize=thread, it shows tons of errors :-(

Confirmed.

> Did you notice similar behavior with
> libgomp/testsuite/libgomp.c-c++-common/allocate-1.c?

No, this one I always saw PASS.  (... which only means so much, of
course...)

> It was used as base for fortran testcase and it
> shows similar warnings with -fthread=sanitize.

Confirmed.

> I am trying to figure out if the problem you observed
> is a general one or just specific to fortran testcase.

So, unless the '-fsanitize=thread' issues are bogus -- unlikely ;-) -- it
seems a latent issue generally, now fatal with
'libgomp.fortran/allocate-1.f90'.


Grüße
 Thomas
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] [gfortran] Add support for allocate clause (OpenMP 5.0).
  2022-01-25  9:19                 ` Thomas Schwinge
@ 2022-01-25 10:32                   ` Tobias Burnus
  2022-01-31 19:13                     ` Hafiz Abid Qadeer
  0 siblings, 1 reply; 21+ messages in thread
From: Tobias Burnus @ 2022-01-25 10:32 UTC (permalink / raw)
  To: Thomas Schwinge, Jakub Jelinek, Hafiz Abid Qadeer; +Cc: gcc-patches, fortran

On 25.01.22 10:19, Thomas Schwinge wrote:
>> I am trying to figure out if the problem you observed
>> is a general one or just specific to fortran testcase.
> So, unless the '-fsanitize=thread' issues are bogus -- unlikely ;-) -- it
> seems a latent issue generally, now fatal with
> 'libgomp.fortran/allocate-1.f90'.

There is one known issue with libgomp and TSAN (-fsanitize=thread)
that I tend to forget about :-(

That's according to Jakub, who wrote a while ago:

"TSAN doesn't understand what libgomp is doing, unless built with --disable-linux-futex"



However, I now tried to disable futex and still get the following.
(First result for libgomp.c-c++-common/allocate-1.c).

On the other hand, I have the feeling that the configure option is
a no op for libgomp. This can also be seen in the configure.ac script,
which only for libstdc++ uses the result and the others have a no-op
call to 'true' (alias ':'):

libgomp/configure.ac:GCC_LINUX_FUTEX(:)
libitm/configure.ac:GCC_LINUX_FUTEX(:)
libstdc++-v3/configure.ac:GCC_LINUX_FUTEX([AC_DEFINE(HAVE_LINUX_FUTEX, 1, [Define if futex syscall is available.])])

(The check is not completely pointless as some checks are still done;
e.g. 'SYS_gettid and SYS_futex required'.)

(TSAN did find issues in libgomp in the past, however. But those
habe been fixed.)


Thus, there might or might not be an issue when TSAN reports one.

  * * *

Glancing at the Fortran testcase, I noted the following,
which probably does not cause the problems. But still,
I want to mention it:

   !$omp parallel private (y, v) firstprivate (x) allocate (x, y, v)
   if (x /= 42) then
     stop 1
   end if

   v(1) = 7
   if ( (and(fl, 2) /= 0) .and.          &
        ((is_64bit_aligned(x) == 0) .or. &
         (is_64bit_aligned(y) == 0) .or. &
         (is_64bit_aligned(v(1)) == 0))) then
       stop 2
   end if

If one compares this with the C/C++ testcase, I note that there
is a barrier before the alignment check in C/C++ but not in
Fortran. Additionally, 'v(1) = 7' is set twice and the
alignment check happens earlier than in C/C++. Not that that
should really matter, but I just saw it.


In C/C++:
   int v[x], w[x];
...
     v[0] = 7;
     v[41] = 8;

In Fortran:
   integer, dimension(x) :: v
...
   v(1) = 7
   v(41) = 8

where 'x == 42'. The Fortran version is not really wrong, but I think
the idea is to set the first and last array element - and that's here
v(42) and not v(41).

BTW: Fortran permits to specify a different lower bound. When converting
C/C++ testcases, it can be useful to use the same lower bound also in
Fortran:   integer :: v(0:x-1)  (or: 'integer, dimension(0:x-1) :: v')
uses then 0 ... 41 for the indices instead of 1 ... 42.

But one has to be careful as Fortran uses the upper bound and C uses the
number of elements. (Same with OpenMP array sections in Fortran vs. C.)


Tobias

PS: The promised data-race warning:
==================
WARNING: ThreadSanitizer: data race (pid=4135381)
   Read of size 8 at 0x7ffc0888bdc0 by thread T10:
     #0 foo._omp_fn.2 libgomp.c-c++-common/allocate-1.c:47 (a.out+0x402c05)
     #1 gomp_thread_start ../../../repos/gcc/libgomp/team.c:129 (libgomp.so.1+0x1e5ed)

   Previous write of size 8 at 0x7ffc0888bdc0 by main thread:
     #0 foo._omp_fn.1 libgomp.c-c++-common/allocate-1.c:47 (a.out+0x402aee)
     #1 GOMP_teams_reg ../../../repos/gcc/libgomp/teams.c:51 (libgomp.so.1+0x3638c)
     #2 main libgomp.c-c++-common/allocate-1.c:366 (a.out+0x40273e)

   Location is stack of main thread.

   Location is global '<null>' at 0x000000000000 ([stack]+0x1ddc0)

   Thread T10 (tid=4135398, running) created by main thread at:
     #0 pthread_create ../../../../repos/gcc/libsanitizer/tsan/tsan_interceptors_posix.cpp:1001 (libtsan.so.2+0x62c76)
     #1 gomp_team_start ../../../repos/gcc/libgomp/team.c:858 (libgomp.so.1+0x1ec18)
     #2 main libgomp.c-c++-common/allocate-1.c:366 (a.out+0x40273e)

SUMMARY: ThreadSanitizer: data race libgomp.c-c++-common/allocate-1.c:47 in foo._omp_fn.2
==================

-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] [gfortran] Add support for allocate clause (OpenMP 5.0).
  2022-01-25 10:32                   ` Tobias Burnus
@ 2022-01-31 19:13                     ` Hafiz Abid Qadeer
  2022-02-04  9:46                       ` Thomas Schwinge
  0 siblings, 1 reply; 21+ messages in thread
From: Hafiz Abid Qadeer @ 2022-01-31 19:13 UTC (permalink / raw)
  To: Tobias Burnus, Thomas Schwinge, Jakub Jelinek, Hafiz Abid Qadeer
  Cc: gcc-patches, fortran

[-- Attachment #1: Type: text/plain, Size: 4581 bytes --]

On 25/01/2022 10:32, Tobias Burnus wrote:
> On 25.01.22 10:19, Thomas Schwinge wrote:
>>> I am trying to figure out if the problem you observed
>>> is a general one or just specific to fortran testcase.
>> So, unless the '-fsanitize=thread' issues are bogus -- unlikely ;-) -- it
>> seems a latent issue generally, now fatal with
>> 'libgomp.fortran/allocate-1.f90'.
> 
> There is one known issue with libgomp and TSAN (-fsanitize=thread)
> that I tend to forget about :-(
> 
> That's according to Jakub, who wrote a while ago:
> 
> "TSAN doesn't understand what libgomp is doing, unless built with --disable-linux-futex"
> 
> 
> 
> However, I now tried to disable futex and still get the following.
> (First result for libgomp.c-c++-common/allocate-1.c).
> 
> On the other hand, I have the feeling that the configure option is
> a no op for libgomp. This can also be seen in the configure.ac script,
> which only for libstdc++ uses the result and the others have a no-op
> call to 'true' (alias ':'):
> 
> libgomp/configure.ac:GCC_LINUX_FUTEX(:)
> libitm/configure.ac:GCC_LINUX_FUTEX(:)
> libstdc++-v3/configure.ac:GCC_LINUX_FUTEX([AC_DEFINE(HAVE_LINUX_FUTEX, 1, [Define if futex syscall
> is available.])])
> 
> (The check is not completely pointless as some checks are still done;
> e.g. 'SYS_gettid and SYS_futex required'.)
> 
> (TSAN did find issues in libgomp in the past, however. But those
> habe been fixed.)
> 
> 
> Thus, there might or might not be an issue when TSAN reports one.
> 
>  * * *
> 
> Glancing at the Fortran testcase, I noted the following,
> which probably does not cause the problems. But still,
> I want to mention it:
> 
>   !$omp parallel private (y, v) firstprivate (x) allocate (x, y, v)
>   if (x /= 42) then
>     stop 1
>   end if
> 
>   v(1) = 7
>   if ( (and(fl, 2) /= 0) .and.          &
>        ((is_64bit_aligned(x) == 0) .or. &
>         (is_64bit_aligned(y) == 0) .or. &
>         (is_64bit_aligned(v(1)) == 0))) then
>       stop 2
>   end if
> 
> If one compares this with the C/C++ testcase, I note that there
> is a barrier before the alignment check in C/C++ but not in
> Fortran. Additionally, 'v(1) = 7' is set twice and the
> alignment check happens earlier than in C/C++. Not that that
> should really matter, but I just saw it.
> 
> 
> In C/C++:
>   int v[x], w[x];
> ...
>     v[0] = 7;
>     v[41] = 8;
> 
> In Fortran:
>   integer, dimension(x) :: v
> ...
>   v(1) = 7
>   v(41) = 8
> 
> where 'x == 42'. The Fortran version is not really wrong, but I think
> the idea is to set the first and last array element - and that's here
> v(42) and not v(41).
> 
> BTW: Fortran permits to specify a different lower bound. When converting
> C/C++ testcases, it can be useful to use the same lower bound also in
> Fortran:   integer :: v(0:x-1)  (or: 'integer, dimension(0:x-1) :: v')
> uses then 0 ... 41 for the indices instead of 1 ... 42.
> 
> But one has to be careful as Fortran uses the upper bound and C uses the
> number of elements. (Same with OpenMP array sections in Fortran vs. C.)
> 
> 
> Tobias
> 
> PS: The promised data-race warning:
> ==================
> WARNING: ThreadSanitizer: data race (pid=4135381)
>   Read of size 8 at 0x7ffc0888bdc0 by thread T10:
>     #0 foo._omp_fn.2 libgomp.c-c++-common/allocate-1.c:47 (a.out+0x402c05)
>     #1 gomp_thread_start ../../../repos/gcc/libgomp/team.c:129 (libgomp.so.1+0x1e5ed)
> 
>   Previous write of size 8 at 0x7ffc0888bdc0 by main thread:
>     #0 foo._omp_fn.1 libgomp.c-c++-common/allocate-1.c:47 (a.out+0x402aee)
>     #1 GOMP_teams_reg ../../../repos/gcc/libgomp/teams.c:51 (libgomp.so.1+0x3638c)
>     #2 main libgomp.c-c++-common/allocate-1.c:366 (a.out+0x40273e)
> 
>   Location is stack of main thread.
> 
>   Location is global '<null>' at 0x000000000000 ([stack]+0x1ddc0)
> 
>   Thread T10 (tid=4135398, running) created by main thread at:
>     #0 pthread_create ../../../../repos/gcc/libsanitizer/tsan/tsan_interceptors_posix.cpp:1001
> (libtsan.so.2+0x62c76)
>     #1 gomp_team_start ../../../repos/gcc/libgomp/team.c:858 (libgomp.so.1+0x1ec18)
>     #2 main libgomp.c-c++-common/allocate-1.c:366 (a.out+0x40273e)
> 
> SUMMARY: ThreadSanitizer: data race libgomp.c-c++-common/allocate-1.c:47 in foo._omp_fn.2
> ==================
> 

Problem was with the pool_size trait. It has limited size which this testcase exceeded. I have
removed it now which seems to fix the problem. Ok to commit the attached patch?

Thanks,
-- 
Hafiz Abid Qadeer
Mentor, a Siemens Business

[-- Attachment #2: 0001-libgomp-Fix-testcase-to-remove-out-of-memory-error.patch --]
[-- Type: text/x-patch, Size: 1711 bytes --]

From 98b5493bd94520dd78b3963d3a4e67cba6bfb6aa Mon Sep 17 00:00:00 2001
From: Hafiz Abid Qadeer <abidh@codesourcery.com>
Date: Mon, 31 Jan 2022 19:02:14 +0000
Subject: [PATCH] [libgomp] Fix testcase to remove out of memory error.

Thomas reported in
https://gcc.gnu.org/pipermail/gcc-patches/2022-January/589039.html
that this testcase is randomly failing. The problem was fixed pool
size which was exhausted when there were a lot of threads. Fixed it
by removing pool_size trait which causes default pool size to be used
which should be big enough.

libgomp/ChangeLog:

	* testsuite/libgomp.fortran/allocate-1.f90: Remove pool_size trait.
---
 libgomp/testsuite/libgomp.fortran/allocate-1.f90 | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/libgomp/testsuite/libgomp.fortran/allocate-1.f90 b/libgomp/testsuite/libgomp.fortran/allocate-1.f90
index 35d1750b878..04bf2307462 100644
--- a/libgomp/testsuite/libgomp.fortran/allocate-1.f90
+++ b/libgomp/testsuite/libgomp.fortran/allocate-1.f90
@@ -313,13 +313,12 @@ program main
   integer, dimension(4) :: p
   integer, dimension(4) :: q
 
-  type (omp_alloctrait) :: traits(3)
+  type (omp_alloctrait) :: traits(2)
   integer (omp_allocator_handle_kind) :: a
 
   traits = [omp_alloctrait (omp_atk_alignment, 64), &
-            omp_alloctrait (omp_atk_fallback, omp_atv_null_fb), &
-            omp_alloctrait (omp_atk_pool_size, 8192)]
-  a = omp_init_allocator (omp_default_mem_space, 3, traits)
+            omp_alloctrait (omp_atk_fallback, omp_atv_null_fb)]
+  a = omp_init_allocator (omp_default_mem_space, 2, traits)
   if (a == omp_null_allocator) stop 1
 
   call omp_set_default_allocator (omp_default_mem_alloc);
-- 
2.25.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] [gfortran] Add support for allocate clause (OpenMP 5.0).
  2022-01-24  8:45             ` Tobias Burnus
  2022-01-24 12:54               ` Hafiz Abid Qadeer
@ 2022-02-04  9:37               ` Thomas Schwinge
  2022-02-04 13:57                 ` [committed] libgomp.fortran/allocate-1.f90: Minor cleanup (was: Re: [PATCH] [gfortran] Add support for allocate clause (OpenMP 5.0).) Tobias Burnus
  1 sibling, 1 reply; 21+ messages in thread
From: Thomas Schwinge @ 2022-02-04  9:37 UTC (permalink / raw)
  To: Tobias Burnus; +Cc: Hafiz Abid Qadeer, Jakub Jelinek, gcc-patches, fortran

Hi Tobias!

On 2022-01-24T09:45:48+0100, Tobias Burnus <tobias@codesourcery.com> wrote:
> On 21.01.22 18:43, Tobias Burnus wrote:
>> On 21.01.22 18:15, Thomas Schwinge wrote:
>>>         11 |     integer(c_int) function is_64bit_aligned (a) bind(C)
>>>      Warning: Variable ‘a’ at (1) is a dummy argument of the BIND(C)
>>> procedure ‘is_64bit_aligned’ but may not be C interoperable
>>> [-Wc-binding-type]
>>>
>>> Is that something to worry about?
> I have attached a patch (not commited), which silences the three kind of
> warnings and fixes the interface issue.
> TODO: commit it.

Still "TODO: commit it" ;-) -- and while I haven't reviewed the changes
in detail, I did spot one item that should be addressed, I suppose:

> --- a/libgomp/testsuite/libgomp.fortran/allocate-1.c
> +++ b/libgomp/testsuite/libgomp.fortran/allocate-1.c
> @@ -1,7 +1,7 @@
>  #include <stdint.h>
>
>  int
> -is_64bit_aligned_ (uintptr_t a)
> +is_64bit_aligned (uintptr_t a)
>  {
>    return ( (a & 0x3f) == 0);
>  }

> --- a/libgomp/testsuite/libgomp.fortran/allocate-1.f90
> +++ b/libgomp/testsuite/libgomp.fortran/allocate-1.f90
> @@ -5,30 +5,30 @@
>  module m
>    use omp_lib
>    use iso_c_binding
> -  implicit none
> +  implicit none (type, external)
>
>    interface
>      integer(c_int) function is_64bit_aligned (a) bind(C)
>        import :: c_int
> -      integer  :: a
> +      type(*)  :: a
>      end
>    end interface
> -end module m
>
> -subroutine foo (x, p, q, px, h, fl)
> +contains
> +
> +subroutine foo (x, p, q, h, fl)
>    use omp_lib
>    use iso_c_binding
>    integer  :: x
>    integer, dimension(4) :: p
>    integer, dimension(4) :: q
> -  integer  :: px
>    integer (kind=omp_allocator_handle_kind) :: h
>    integer  :: fl
>
>    integer  :: y
>    integer  :: r, i, i1, i2, i3, i4, i5
>    integer  :: l, l3, l4, l5, l6
> -  integer  :: n, n1, n2, n3, n4
> +  integer  :: n, n2, n3, n4
>    integer  :: j2, j3, j4
>    integer, dimension(4) :: l2
>    integer, dimension(4) :: r2
> @@ -118,6 +118,7 @@ subroutine foo (x, p, q, px, h, fl)
>    end if
>    !$omp end parallel
>    !$omp end teams
> +stop
>
>    !$omp parallel do private (y) firstprivate (x)  reduction(+: r) allocate (h: x, y, r, l, n) lastprivate (l)  linear (n: 16)
>    do i = 0, 63

That early 'stop' should probably be backed out?  ;-)


Grüße
 Thomas


> @@ -153,77 +154,77 @@ subroutine foo (x, p, q, px, h, fl)
>             ((is_64bit_aligned(l2(1)) == 0) .or. &
>              (is_64bit_aligned(l3) == 0) .or. &
>              (is_64bit_aligned(i1) == 0))) then
> -     stop 10
> +        stop 10
>        end if
>      end do
>
>      !$omp do collapse(2) lastprivate(l4, i2, j2) linear (n2:17) allocate (h: n2, l4, i2, j2)
>      do i2 = 3, 4
>        do j2 = 17, 22, 2
> -     n2 = n2 + 17
> -     l4 = i2 * 31 + j2
> -     if ( (and(fl, 1) /= 0) .and.          &
> -       ((is_64bit_aligned(l4) == 0) .or. &
> -       (is_64bit_aligned(n2) == 0) .or. &
> -       (is_64bit_aligned(i2) == 0) .or. &
> -       (is_64bit_aligned(j2) == 0))) then
> -       stop 11
> -     end if
> +        n2 = n2 + 17
> +        l4 = i2 * 31 + j2
> +        if ( (and(fl, 1) /= 0) .and.          &
> +             ((is_64bit_aligned(l4) == 0) .or. &
> +              (is_64bit_aligned(n2) == 0) .or. &
> +              (is_64bit_aligned(i2) == 0) .or. &
> +              (is_64bit_aligned(j2) == 0))) then
> +          stop 11
> +        end if
>        end do
>      end do
>
>      !$omp do collapse(2) lastprivate(l5, i3, j3) linear (n3:17) schedule (static, 3) allocate (n3, l5, i3, j3)
>      do i3 = 3, 4
>        do j3 = 17, 22, 2
> -       n3 = n3 + 17
> -       l5 = i3 * 31 + j3
> -       if ( (and(fl, 2) /= 0) .and.      &
> -       ((is_64bit_aligned(l5) == 0) .or. &
> -       (is_64bit_aligned(n3) == 0) .or. &
> -       (is_64bit_aligned(i3) == 0) .or. &
> -       (is_64bit_aligned(j3) == 0))) then
> -       stop 12
> -     end if
> +          n3 = n3 + 17
> +          l5 = i3 * 31 + j3
> +          if ( (and(fl, 2) /= 0) .and.      &
> +             ((is_64bit_aligned(l5) == 0) .or. &
> +              (is_64bit_aligned(n3) == 0) .or. &
> +              (is_64bit_aligned(i3) == 0) .or. &
> +              (is_64bit_aligned(j3) == 0))) then
> +          stop 12
> +        end if
>        end do
>      end do
>
>      !$omp do collapse(2) lastprivate(l6, i4, j4) linear (n4:17) schedule (dynamic) allocate (h: n4, l6, i4, j4)
>      do i4 = 3, 4
>        do j4 = 17, 22,2
> -       n4 = n4 + 17;
> -       l6 = i4 * 31 + j4;
> -     if ( (and(fl, 1) /= 0) .and.          &
> -       ((is_64bit_aligned(l6) == 0) .or. &
> -       (is_64bit_aligned(n4) == 0) .or. &
> -       (is_64bit_aligned(i4) == 0) .or. &
> -       (is_64bit_aligned(j4) == 0))) then
> -       stop 13
> -     end if
> +          n4 = n4 + 17;
> +          l6 = i4 * 31 + j4;
> +        if ( (and(fl, 1) /= 0) .and.          &
> +            ((is_64bit_aligned(l6) == 0) .or. &
> +             (is_64bit_aligned(n4) == 0) .or. &
> +             (is_64bit_aligned(i4) == 0) .or. &
> +             (is_64bit_aligned(j4) == 0))) then
> +          stop 13
> +        end if
>        end do
>      end do
>
>      !$omp do lastprivate (i5) allocate (i5)
>      do i5 = 1, 17, 3
>        if ( (and(fl, 2) /= 0) .and.          &
> -        (is_64bit_aligned(i5) == 0)) then
> -     stop 14
> +           (is_64bit_aligned(i5) == 0)) then
> +        stop 14
>        end if
>      end do
>
>      !$omp do reduction(+:p, q, r2) allocate(h: p, q, r2)
>      do i = 0, 31
> -     p(3) = p(3) +  i;
> -     p(4) = p(4) + (2 * i)
> -     q(1) = q(1) + (3 * i)
> -     q(3) = q(3) + (4 * i)
> -     r2(1) = r2(1) + (5 * i)
> -     r2(4) = r2(4) + (6 * i)
> -     if ( (and(fl, 1) /= 0) .and.          &
> -       ((is_64bit_aligned(q(1)) == 0) .or. &
> -       (is_64bit_aligned(p(1)) == 0) .or. &
> -       (is_64bit_aligned(r2(1)) == 0) )) then
> -       stop 15
> -     end if
> +        p(3) = p(3) +  i;
> +        p(4) = p(4) + (2 * i)
> +        q(1) = q(1) + (3 * i)
> +        q(3) = q(3) + (4 * i)
> +        r2(1) = r2(1) + (5 * i)
> +        r2(4) = r2(4) + (6 * i)
> +        if ( (and(fl, 1) /= 0) .and.             &
> +             ((is_64bit_aligned(q(1)) == 0) .or. &
> +              (is_64bit_aligned(p(1)) == 0) .or. &
> +              (is_64bit_aligned(r2(1)) == 0) )) then
> +          stop 15
> +        end if
>      end do
>
>      !$omp task private(y) firstprivate(x) allocate(x, y)
> @@ -305,11 +306,13 @@ subroutine foo (x, p, q, px, h, fl)
>        .or. r2(1) /= (5 * p(3)) .or. r2(4) /= (6 * p(3))) then
>      stop 25
>    end if
> -
>  end subroutine
> +end module m
>
>  program main
>    use omp_lib
> +  use m
> +  implicit none (type, external)
>    integer, dimension(4) :: p
>    integer, dimension(4) :: q
>
> @@ -323,11 +326,11 @@ program main
>    if (a == omp_null_allocator) stop 1
>
>    call omp_set_default_allocator (omp_default_mem_alloc);
> -  call foo (42, p, q, 2, a, 0);
> -  call foo (42, p, q, 2, omp_default_mem_alloc, 0);
> -  call foo (42, p, q, 2, a, 1);
> +  call foo (42, p, q, a, 0);
> +  call foo (42, p, q, omp_default_mem_alloc, 0);
> +  call foo (42, p, q, a, 1);
>    call omp_set_default_allocator (a);
> -  call foo (42, p, q, 2, omp_null_allocator, 3);
> -  call foo (42, p, q, 2, omp_default_mem_alloc, 2);
> +  call foo (42, p, q, omp_null_allocator, 3);
> +  call foo (42, p, q, omp_default_mem_alloc, 2);
>    call omp_destroy_allocator (a);
>  end
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] [gfortran] Add support for allocate clause (OpenMP 5.0).
  2022-01-31 19:13                     ` Hafiz Abid Qadeer
@ 2022-02-04  9:46                       ` Thomas Schwinge
  2022-02-04 11:25                         ` Hafiz Abid Qadeer
  0 siblings, 1 reply; 21+ messages in thread
From: Thomas Schwinge @ 2022-02-04  9:46 UTC (permalink / raw)
  To: Hafiz Abid Qadeer, Tobias Burnus, Jakub Jelinek; +Cc: gcc-patches, fortran

Hi!

On 2022-01-31T19:13:09+0000, Hafiz Abid Qadeer <abid_qadeer@mentor.com> wrote:
> On 25/01/2022 10:32, Tobias Burnus wrote:
>> On 25.01.22 10:19, Thomas Schwinge wrote:
>>>> I am trying to figure out if the problem you observed
>>>> is a general one or just specific to fortran testcase.
>>> So, unless the '-fsanitize=thread' issues are bogus -- unlikely ;-) -- it
>>> seems a latent issue generally, now fatal with
>>> 'libgomp.fortran/allocate-1.f90'.
>>
>> There is one known issue with libgomp and TSAN (-fsanitize=thread)
>> that I tend to forget about :-(
>>
>> That's according to Jakub, who wrote a while ago:
>>
>> "TSAN doesn't understand what libgomp is doing, unless built with --disable-linux-futex"

Uh.  Anything that can reasonably be done to address this?  At least, to
make this obvious to the user of '-fsanitize=thread'?

>> However, I now tried to disable futex and still get the following.
>> (First result for libgomp.c-c++-common/allocate-1.c).
>>
>> On the other hand, I have the feeling that the configure option is
>> a no op for libgomp. This can also be seen in the configure.ac script,
>> which only for libstdc++ uses the result and the others have a no-op
>> call to 'true' (alias ':'):
>>
>> libgomp/configure.ac:GCC_LINUX_FUTEX(:)
>> libitm/configure.ac:GCC_LINUX_FUTEX(:)
>> libstdc++-v3/configure.ac:GCC_LINUX_FUTEX([AC_DEFINE(HAVE_LINUX_FUTEX, 1, [Define if futex syscall
>> is available.])])
>>
>> (The check is not completely pointless as some checks are still done;
>> e.g. 'SYS_gettid and SYS_futex required'.)

Uh.  That (make '--disable-linux-futex' work) should be fixed, I suppose?

>> (TSAN did find issues in libgomp in the past, however. But those
>> habe been fixed.)
>>
>>
>> Thus, there might or might not be an issue when TSAN reports one.
>>
>>  * * *
>>
>> Glancing at the Fortran testcase, I noted the following,
>> which probably does not cause the problems. But still,
>> I want to mention it:
>>
>>   !$omp parallel private (y, v) firstprivate (x) allocate (x, y, v)
>>   if (x /= 42) then
>>     stop 1
>>   end if
>>
>>   v(1) = 7
>>   if ( (and(fl, 2) /= 0) .and.          &
>>        ((is_64bit_aligned(x) == 0) .or. &
>>         (is_64bit_aligned(y) == 0) .or. &
>>         (is_64bit_aligned(v(1)) == 0))) then
>>       stop 2
>>   end if
>>
>> If one compares this with the C/C++ testcase, I note that there
>> is a barrier before the alignment check in C/C++ but not in
>> Fortran. Additionally, 'v(1) = 7' is set twice and the
>> alignment check happens earlier than in C/C++. Not that that
>> should really matter, but I just saw it.
>>
>>
>> In C/C++:
>>   int v[x], w[x];
>> ...
>>     v[0] = 7;
>>     v[41] = 8;
>>
>> In Fortran:
>>   integer, dimension(x) :: v
>> ...
>>   v(1) = 7
>>   v(41) = 8
>>
>> where 'x == 42'. The Fortran version is not really wrong, but I think
>> the idea is to set the first and last array element - and that's here
>> v(42) and not v(41).
>>
>> BTW: Fortran permits to specify a different lower bound. When converting
>> C/C++ testcases, it can be useful to use the same lower bound also in
>> Fortran:   integer :: v(0:x-1)  (or: 'integer, dimension(0:x-1) :: v')
>> uses then 0 ... 41 for the indices instead of 1 ... 42.
>>
>> But one has to be careful as Fortran uses the upper bound and C uses the
>> number of elements. (Same with OpenMP array sections in Fortran vs. C.)

Abid, are you going to address these?  I think it does make sense if the
C/C++ and Fortran test cases match as much as feasible.

>> PS: The promised data-race warning:
>> ==================
>> WARNING: ThreadSanitizer: data race (pid=4135381)
>>   Read of size 8 at 0x7ffc0888bdc0 by thread T10:
>>     #0 foo._omp_fn.2 libgomp.c-c++-common/allocate-1.c:47 (a.out+0x402c05)
>>     #1 gomp_thread_start ../../../repos/gcc/libgomp/team.c:129 (libgomp.so.1+0x1e5ed)
>>
>>   Previous write of size 8 at 0x7ffc0888bdc0 by main thread:
>>     #0 foo._omp_fn.1 libgomp.c-c++-common/allocate-1.c:47 (a.out+0x402aee)
>>     #1 GOMP_teams_reg ../../../repos/gcc/libgomp/teams.c:51 (libgomp.so.1+0x3638c)
>>     #2 main libgomp.c-c++-common/allocate-1.c:366 (a.out+0x40273e)
>>
>>   Location is stack of main thread.
>>
>>   Location is global '<null>' at 0x000000000000 ([stack]+0x1ddc0)
>>
>>   Thread T10 (tid=4135398, running) created by main thread at:
>>     #0 pthread_create ../../../../repos/gcc/libsanitizer/tsan/tsan_interceptors_posix.cpp:1001
>> (libtsan.so.2+0x62c76)
>>     #1 gomp_team_start ../../../repos/gcc/libgomp/team.c:858 (libgomp.so.1+0x1ec18)
>>     #2 main libgomp.c-c++-common/allocate-1.c:366 (a.out+0x40273e)
>>
>> SUMMARY: ThreadSanitizer: data race libgomp.c-c++-common/allocate-1.c:47 in foo._omp_fn.2
>> ==================
>>
>
> Problem was with the pool_size trait. It has limited size which this testcase exceeded. I have
> removed it now which seems to fix the problem. Ok to commit the attached patch?

First, I do confirm that this (testing together with Tobias' patch "which
silences the three kind of warnings and fixes the interface issue" plus
my suggestion that the "early 'stop' [...] be backed out") does resolve
the execution FAILs, thanks.

However: really (a) remove 'omp_alloctrait (omp_atk_pool_size, 8192)'
altogether, or instead: (b) increase its size (if that can be computed)
-- and/or (c) limit the number of OpenMP threads executing in parallel?
Due to unfamiliarity with all that, I don't know what's best here.


Grüße
 Thomas


> From 98b5493bd94520dd78b3963d3a4e67cba6bfb6aa Mon Sep 17 00:00:00 2001
> From: Hafiz Abid Qadeer <abidh@codesourcery.com>
> Date: Mon, 31 Jan 2022 19:02:14 +0000
> Subject: [PATCH] [libgomp] Fix testcase to remove out of memory error.
>
> Thomas reported in
> https://gcc.gnu.org/pipermail/gcc-patches/2022-January/589039.html
> that this testcase is randomly failing. The problem was fixed pool
> size which was exhausted when there were a lot of threads. Fixed it
> by removing pool_size trait which causes default pool size to be used
> which should be big enough.
>
> libgomp/ChangeLog:
>
>       * testsuite/libgomp.fortran/allocate-1.f90: Remove pool_size trait.
> ---
>  libgomp/testsuite/libgomp.fortran/allocate-1.f90 | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/libgomp/testsuite/libgomp.fortran/allocate-1.f90 b/libgomp/testsuite/libgomp.fortran/allocate-1.f90
> index 35d1750b878..04bf2307462 100644
> --- a/libgomp/testsuite/libgomp.fortran/allocate-1.f90
> +++ b/libgomp/testsuite/libgomp.fortran/allocate-1.f90
> @@ -313,13 +313,12 @@ program main
>    integer, dimension(4) :: p
>    integer, dimension(4) :: q
>
> -  type (omp_alloctrait) :: traits(3)
> +  type (omp_alloctrait) :: traits(2)
>    integer (omp_allocator_handle_kind) :: a
>
>    traits = [omp_alloctrait (omp_atk_alignment, 64), &
> -            omp_alloctrait (omp_atk_fallback, omp_atv_null_fb), &
> -            omp_alloctrait (omp_atk_pool_size, 8192)]
> -  a = omp_init_allocator (omp_default_mem_space, 3, traits)
> +            omp_alloctrait (omp_atk_fallback, omp_atv_null_fb)]
> +  a = omp_init_allocator (omp_default_mem_space, 2, traits)
>    if (a == omp_null_allocator) stop 1
>
>    call omp_set_default_allocator (omp_default_mem_alloc);
> --
> 2.25.1
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] [gfortran] Add support for allocate clause (OpenMP 5.0).
  2022-02-04  9:46                       ` Thomas Schwinge
@ 2022-02-04 11:25                         ` Hafiz Abid Qadeer
  2022-02-05 19:09                           ` Hafiz Abid Qadeer
  0 siblings, 1 reply; 21+ messages in thread
From: Hafiz Abid Qadeer @ 2022-02-04 11:25 UTC (permalink / raw)
  To: Thomas Schwinge, Hafiz Abid Qadeer, Tobias Burnus, Jakub Jelinek
  Cc: gcc-patches, fortran

On 04/02/2022 09:46, Thomas Schwinge wrote:

> 
> Abid, are you going to address these?  I think it does make sense if the
> C/C++ and Fortran test cases match as much as feasible.
> 
Sure. I will do that.

> However: really (a) remove 'omp_alloctrait (omp_atk_pool_size, 8192)'
> altogether, or instead: (b) increase its size (if that can be computed)
> -- and/or (c) limit the number of OpenMP threads executing in parallel?
> Due to unfamiliarity with all that, I don't know what's best here.
> 
C testcase also does not have the pool_size trait. So it makes sense to me to not have it in fortran
testcase too. It also seems more cleaner than putting some limits on number of threads or increasing
the size which will be a bit fragile.

Thanks,
-- 
Hafiz Abid Qadeer


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [committed] libgomp.fortran/allocate-1.f90: Minor cleanup (was: Re: [PATCH] [gfortran] Add support for allocate clause (OpenMP 5.0).)
  2022-02-04  9:37               ` Thomas Schwinge
@ 2022-02-04 13:57                 ` Tobias Burnus
  2022-02-04 15:33                   ` Thomas Schwinge
  0 siblings, 1 reply; 21+ messages in thread
From: Tobias Burnus @ 2022-02-04 13:57 UTC (permalink / raw)
  To: Thomas Schwinge, Tobias Burnus
  Cc: Hafiz Abid Qadeer, Jakub Jelinek, gcc-patches, fortran

[-- Attachment #1: Type: text/plain, Size: 1025 bytes --]

Hi Thomas,

On 04.02.22 10:37, Thomas Schwinge wrote:
>> I have attached a patch (not commited), which silences the three kind of
>> warnings and fixes the interface issue.
>> TODO: commit it.
> Still "TODO: commit it" ;-) -- and while I haven't reviewed the changes
> in detail, I did spot one item that should be addressed, I suppose:

I had also spotted the 'stop' which was a left over from -fsanitized=...
checking and had removed it locally. But good that you also keep
checking patches :-)

In any case, I have now _finally_ committed the patch.

Attached is the simplified (-w) diff, where I did exclude the
indentation changes to make the diff more readable.

For the full diff, see e.g. https://gcc.gnu.org/r12-7053

Tobias
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

[-- Attachment #2: committed.diff --]
[-- Type: text/x-patch, Size: 3315 bytes --]

commit 6d4981350168f1eb3f72149bd7e05b9ba6bec1fd
Author: Tobias Burnus <tobias@codesourcery.com>
Date:   Fri Feb 4 14:51:01 2022 +0100

    libgomp.fortran/allocate-1.f90: Minor cleanup
    
    libgomp/ChangeLog:
            * testsuite/libgomp.fortran/allocate-1.c (is_64bit_aligned): Renamed
            from is_64bit_aligned_.
            * testsuite/libgomp.fortran/allocate-1.f90: Fix interface decl
            and use it, more implicit none, remove unused argument.

diff --git a/libgomp/testsuite/libgomp.fortran/allocate-1.c b/libgomp/testsuite/libgomp.fortran/allocate-1.c
index d33acc6feef..cb6d355afc6 100644
--- a/libgomp/testsuite/libgomp.fortran/allocate-1.c
+++ b/libgomp/testsuite/libgomp.fortran/allocate-1.c
@@ -1,7 +1,7 @@
 #include <stdint.h>
 
 int
-is_64bit_aligned_ (uintptr_t a)
+is_64bit_aligned (uintptr_t a)
 {
   return ( (a & 0x3f) == 0);
 }
diff --git a/libgomp/testsuite/libgomp.fortran/allocate-1.f90 b/libgomp/testsuite/libgomp.fortran/allocate-1.f90
index 35d1750b878..062278f9908 100644
--- a/libgomp/testsuite/libgomp.fortran/allocate-1.f90
+++ b/libgomp/testsuite/libgomp.fortran/allocate-1.f90
@@ -5,30 +5,30 @@
 module m
   use omp_lib
   use iso_c_binding
-  implicit none
+  implicit none (type, external)
 
   interface
     integer(c_int) function is_64bit_aligned (a) bind(C)
       import :: c_int
-      integer  :: a
+      type(*)  :: a
     end
   end interface
-end module m
 
-subroutine foo (x, p, q, px, h, fl)
+contains
+
+subroutine foo (x, p, q, h, fl)
   use omp_lib
   use iso_c_binding
   integer  :: x
   integer, dimension(4) :: p
   integer, dimension(4) :: q
-  integer  :: px
   integer (kind=omp_allocator_handle_kind) :: h
   integer  :: fl
 
   integer  :: y
   integer  :: r, i, i1, i2, i3, i4, i5
   integer  :: l, l3, l4, l5, l6
-  integer  :: n, n1, n2, n3, n4
+  integer  :: n, n2, n3, n4
   integer  :: j2, j3, j4
   integer, dimension(4) :: l2
   integer, dimension(4) :: r2
@@ -74,6 +74,8 @@ subroutine foo (x, p, q, px, h, fl)
   if (x /= 42) then
     stop 1
   end if
+
+  !!$omp barrier
   v(1) = 7
   if ( (and(fl, 2) /= 0) .and.          &
        ((is_64bit_aligned(x) == 0) .or. &
@@ -95,7 +97,7 @@ subroutine foo (x, p, q, px, h, fl)
     stop 4
   end if
   !$omp end parallel
-
+stop
   !$omp teams
   !$omp parallel private (y) firstprivate (x, w) allocate (h: x, y, w)
 
@@ -305,11 +307,13 @@ subroutine foo (x, p, q, px, h, fl)
       .or. r2(1) /= (5 * p(3)) .or. r2(4) /= (6 * p(3))) then
     stop 25
   end if
-
 end subroutine
+end module m
 
 program main
   use omp_lib
+  use m
+  implicit none (type, external)
   integer, dimension(4) :: p
   integer, dimension(4) :: q
 
@@ -323,11 +327,11 @@ program main
   if (a == omp_null_allocator) stop 1
 
   call omp_set_default_allocator (omp_default_mem_alloc);
-  call foo (42, p, q, 2, a, 0);
-  call foo (42, p, q, 2, omp_default_mem_alloc, 0);
-  call foo (42, p, q, 2, a, 1);
+  call foo (42, p, q, a, 0);
+  call foo (42, p, q, omp_default_mem_alloc, 0);
+  call foo (42, p, q, a, 1);
   call omp_set_default_allocator (a);
-  call foo (42, p, q, 2, omp_null_allocator, 3);
-  call foo (42, p, q, 2, omp_default_mem_alloc, 2);
+  call foo (42, p, q, omp_null_allocator, 3);
+  call foo (42, p, q, omp_default_mem_alloc, 2);
   call omp_destroy_allocator (a);
 end

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [committed] libgomp.fortran/allocate-1.f90: Minor cleanup (was: Re: [PATCH] [gfortran] Add support for allocate clause (OpenMP 5.0).)
  2022-02-04 13:57                 ` [committed] libgomp.fortran/allocate-1.f90: Minor cleanup (was: Re: [PATCH] [gfortran] Add support for allocate clause (OpenMP 5.0).) Tobias Burnus
@ 2022-02-04 15:33                   ` Thomas Schwinge
  2022-02-04 16:34                     ` Tobias Burnus
  0 siblings, 1 reply; 21+ messages in thread
From: Thomas Schwinge @ 2022-02-04 15:33 UTC (permalink / raw)
  To: Tobias Burnus; +Cc: Hafiz Abid Qadeer, Jakub Jelinek, gcc-patches, fortran

Hi Tobias!

On 2022-02-04T14:57:07+0100, Tobias Burnus <tobias@codesourcery.com> wrote:
> On 04.02.22 10:37, Thomas Schwinge wrote:
>>> I have attached a patch (not commited), which silences the three kind of
>>> warnings and fixes the interface issue.
>>> TODO: commit it.
>> Still "TODO: commit it" ;-) -- and while I haven't reviewed the changes
>> in detail, I did spot one item that should be addressed, I suppose:
>
> I had also spotted the 'stop' which was a left over from -fsanitized=...
> checking and had removed it locally.

Maybe removed locally, I can't tell ;-) -- but it's still in the commit
that you pushed.  See below.

Also, a commented-out '!$omp barrier'; not sure what that one is about.

> But good that you also keep
> checking patches :-)

I try!  :-)


Grüße
 Thomas


> In any case, I have now _finally_ committed the patch.
>
> Attached is the simplified (-w) diff, where I did exclude the
> indentation changes to make the diff more readable.
>
> For the full diff, see e.g. https://gcc.gnu.org/r12-7053
>
> Tobias

> commit 6d4981350168f1eb3f72149bd7e05b9ba6bec1fd
> Author: Tobias Burnus <tobias@codesourcery.com>
> Date:   Fri Feb 4 14:51:01 2022 +0100
>
>     libgomp.fortran/allocate-1.f90: Minor cleanup
>
>     libgomp/ChangeLog:
>             * testsuite/libgomp.fortran/allocate-1.c (is_64bit_aligned): Renamed
>             from is_64bit_aligned_.
>             * testsuite/libgomp.fortran/allocate-1.f90: Fix interface decl
>             and use it, more implicit none, remove unused argument.
>
> diff --git a/libgomp/testsuite/libgomp.fortran/allocate-1.c b/libgomp/testsuite/libgomp.fortran/allocate-1.c
> index d33acc6feef..cb6d355afc6 100644
> --- a/libgomp/testsuite/libgomp.fortran/allocate-1.c
> +++ b/libgomp/testsuite/libgomp.fortran/allocate-1.c
> @@ -1,7 +1,7 @@
>  #include <stdint.h>
>
>  int
> -is_64bit_aligned_ (uintptr_t a)
> +is_64bit_aligned (uintptr_t a)
>  {
>    return ( (a & 0x3f) == 0);
>  }
> diff --git a/libgomp/testsuite/libgomp.fortran/allocate-1.f90 b/libgomp/testsuite/libgomp.fortran/allocate-1.f90
> index 35d1750b878..062278f9908 100644
> --- a/libgomp/testsuite/libgomp.fortran/allocate-1.f90
> +++ b/libgomp/testsuite/libgomp.fortran/allocate-1.f90
> @@ -5,30 +5,30 @@
>  module m
>    use omp_lib
>    use iso_c_binding
> -  implicit none
> +  implicit none (type, external)
>
>    interface
>      integer(c_int) function is_64bit_aligned (a) bind(C)
>        import :: c_int
> -      integer  :: a
> +      type(*)  :: a
>      end
>    end interface
> -end module m
>
> -subroutine foo (x, p, q, px, h, fl)
> +contains
> +
> +subroutine foo (x, p, q, h, fl)
>    use omp_lib
>    use iso_c_binding
>    integer  :: x
>    integer, dimension(4) :: p
>    integer, dimension(4) :: q
> -  integer  :: px
>    integer (kind=omp_allocator_handle_kind) :: h
>    integer  :: fl
>
>    integer  :: y
>    integer  :: r, i, i1, i2, i3, i4, i5
>    integer  :: l, l3, l4, l5, l6
> -  integer  :: n, n1, n2, n3, n4
> +  integer  :: n, n2, n3, n4
>    integer  :: j2, j3, j4
>    integer, dimension(4) :: l2
>    integer, dimension(4) :: r2
> @@ -74,6 +74,8 @@ subroutine foo (x, p, q, px, h, fl)
>    if (x /= 42) then
>      stop 1
>    end if
> +
> +  !!$omp barrier
>    v(1) = 7
>    if ( (and(fl, 2) /= 0) .and.          &
>         ((is_64bit_aligned(x) == 0) .or. &
> @@ -95,7 +97,7 @@ subroutine foo (x, p, q, px, h, fl)
>      stop 4
>    end if
>    !$omp end parallel
> -
> +stop
>    !$omp teams
>    !$omp parallel private (y) firstprivate (x, w) allocate (h: x, y, w)
>
> @@ -305,11 +307,13 @@ subroutine foo (x, p, q, px, h, fl)
>        .or. r2(1) /= (5 * p(3)) .or. r2(4) /= (6 * p(3))) then
>      stop 25
>    end if
> -
>  end subroutine
> +end module m
>
>  program main
>    use omp_lib
> +  use m
> +  implicit none (type, external)
>    integer, dimension(4) :: p
>    integer, dimension(4) :: q
>
> @@ -323,11 +327,11 @@ program main
>    if (a == omp_null_allocator) stop 1
>
>    call omp_set_default_allocator (omp_default_mem_alloc);
> -  call foo (42, p, q, 2, a, 0);
> -  call foo (42, p, q, 2, omp_default_mem_alloc, 0);
> -  call foo (42, p, q, 2, a, 1);
> +  call foo (42, p, q, a, 0);
> +  call foo (42, p, q, omp_default_mem_alloc, 0);
> +  call foo (42, p, q, a, 1);
>    call omp_set_default_allocator (a);
> -  call foo (42, p, q, 2, omp_null_allocator, 3);
> -  call foo (42, p, q, 2, omp_default_mem_alloc, 2);
> +  call foo (42, p, q, omp_null_allocator, 3);
> +  call foo (42, p, q, omp_default_mem_alloc, 2);
>    call omp_destroy_allocator (a);
>  end
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [committed] libgomp.fortran/allocate-1.f90: Minor cleanup (was: Re: [PATCH] [gfortran] Add support for allocate clause (OpenMP 5.0).)
  2022-02-04 15:33                   ` Thomas Schwinge
@ 2022-02-04 16:34                     ` Tobias Burnus
  0 siblings, 0 replies; 21+ messages in thread
From: Tobias Burnus @ 2022-02-04 16:34 UTC (permalink / raw)
  To: Thomas Schwinge, Tobias Burnus
  Cc: Hafiz Abid Qadeer, Jakub Jelinek, gcc-patches, fortran

[-- Attachment #1: Type: text/plain, Size: 644 bytes --]

On 04.02.22 16:33, Thomas Schwinge wrote:
> Maybe removed locally, I can't tell ;-) -- but it's still in the
> commit that you pushed. See below.
> Also, a commented-out '!$omp barrier'; not sure what that one is about.

I shall not do commits after one week of 6h+/day virtual OpenMP
Face2Face meeting.

Corrected with commit as shown in the attachment.

Tobias
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

[-- Attachment #2: committed.diff --]
[-- Type: text/x-patch, Size: 988 bytes --]

commit f62156eab7b757d1ee03a11d5c96c72bd3de079c
Author: Tobias Burnus <tobias@codesourcery.com>
Date:   Fri Feb 4 17:31:21 2022 +0100

    libgomp.fortran/allocate-1.f90: Fix minor cleanup
    
    libgomp/ChangeLog:
            * testsuite/libgomp.fortran/allocate-1.f90: Remove spurious
            STOP of previous commit.

diff --git a/libgomp/testsuite/libgomp.fortran/allocate-1.f90 b/libgomp/testsuite/libgomp.fortran/allocate-1.f90
index 062278f9908..0a31d35d5ac 100644
--- a/libgomp/testsuite/libgomp.fortran/allocate-1.f90
+++ b/libgomp/testsuite/libgomp.fortran/allocate-1.f90
@@ -74,8 +74,6 @@ subroutine foo (x, p, q, h, fl)
   if (x /= 42) then
     stop 1
   end if
-
-  !!$omp barrier
   v(1) = 7
   if ( (and(fl, 2) /= 0) .and.          &
        ((is_64bit_aligned(x) == 0) .or. &
@@ -97,7 +95,6 @@ subroutine foo (x, p, q, h, fl)
     stop 4
   end if
   !$omp end parallel
-stop
   !$omp teams
   !$omp parallel private (y) firstprivate (x, w) allocate (h: x, y, w)
 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] [gfortran] Add support for allocate clause (OpenMP 5.0).
  2022-02-04 11:25                         ` Hafiz Abid Qadeer
@ 2022-02-05 19:09                           ` Hafiz Abid Qadeer
  2022-02-16 10:29                             ` Hafiz Abid Qadeer
  0 siblings, 1 reply; 21+ messages in thread
From: Hafiz Abid Qadeer @ 2022-02-05 19:09 UTC (permalink / raw)
  To: Thomas Schwinge, Tobias Burnus, Jakub Jelinek; +Cc: gcc-patches, fortran

[-- Attachment #1: Type: text/plain, Size: 367 bytes --]

On 04/02/2022 11:25, Hafiz Abid Qadeer wrote:
> On 04/02/2022 09:46, Thomas Schwinge wrote:
> 
>>
>> Abid, are you going to address these?  I think it does make sense if the
>> C/C++ and Fortran test cases match as much as feasible.
>>
> Sure. I will do that.

The attached patch address those issues apart from removing pool_size trait.

Thanks
-- 
Hafiz Abid Qadeer

[-- Attachment #2: 0001-libgomp-Fix-multiple-issue-in-the-testcase-allocate-.patch --]
[-- Type: text/x-patch, Size: 3027 bytes --]

From 7b4dbd5b7c853f0165436ef58339663edce802d5 Mon Sep 17 00:00:00 2001
From: Hafiz Abid Qadeer <abidh@codesourcery.com>
Date: Mon, 31 Jan 2022 19:02:14 +0000
Subject: [PATCH] [libgomp] Fix multiple issue in the testcase allocate-1.f90.

1. Thomas reported in
https://gcc.gnu.org/pipermail/gcc-patches/2022-January/589039.html
that this testcase is randomly failing. The problem was fixed pool
size which was exhausted when there were a lot of threads. Fixed it
by removing pool_size trait which causes default pool size to be used
which should be big enough.

2. Array indices have been changed to check the last element in the
array.

3. Remove a redundant assignment and move some code to better match
C testcase.

libgomp/ChangeLog:

	* testsuite/libgomp.fortran/allocate-1.f90: Remove pool_size
	trait.  Test last index in w and v array.  Remove redundant
	assignment to V(1).  Move alignment checks at the end of
	parallel region.
---
 .../testsuite/libgomp.fortran/allocate-1.f90  | 26 +++++++++----------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/libgomp/testsuite/libgomp.fortran/allocate-1.f90 b/libgomp/testsuite/libgomp.fortran/allocate-1.f90
index 0a31d35d5ac..1547d2baeef 100644
--- a/libgomp/testsuite/libgomp.fortran/allocate-1.f90
+++ b/libgomp/testsuite/libgomp.fortran/allocate-1.f90
@@ -74,31 +74,30 @@ subroutine foo (x, p, q, h, fl)
   if (x /= 42) then
     stop 1
   end if
-  v(1) = 7
-  if ( (and(fl, 2) /= 0) .and.          &
-       ((is_64bit_aligned(x) == 0) .or. &
-        (is_64bit_aligned(y) == 0) .or. &
-        (is_64bit_aligned(v(1)) == 0))) then
-      stop 2
-  end if
 
   !$omp barrier
   y = 1;
   x = x + 1
   v(1) = 7
-  v(41) = 8
+  v(42) = 8
   !$omp barrier
   if (x /= 43 .or. y /= 1) then
     stop 3
   end if
-  if (v(1) /= 7 .or. v(41) /= 8) then
+  if (v(1) /= 7 .or. v(42) /= 8) then
     stop 4
   end if
+  if ( (and(fl, 2) /= 0) .and.        &
+     ((is_64bit_aligned(x) == 0) .or. &
+      (is_64bit_aligned(y) == 0) .or. &
+      (is_64bit_aligned(v(1)) == 0))) then
+    stop 2
+  end if
   !$omp end parallel
   !$omp teams
   !$omp parallel private (y) firstprivate (x, w) allocate (h: x, y, w)
 
-  if (x /= 42 .or. w(17) /= 17 .or. w(41) /= 41) then
+  if (x /= 42 .or. w(17) /= 17 .or. w(42) /= 42) then
     stop 5
   end if
   !$omp barrier
@@ -314,13 +313,12 @@ program main
   integer, dimension(4) :: p
   integer, dimension(4) :: q
 
-  type (omp_alloctrait) :: traits(3)
+  type (omp_alloctrait) :: traits(2)
   integer (omp_allocator_handle_kind) :: a
 
   traits = [omp_alloctrait (omp_atk_alignment, 64), &
-            omp_alloctrait (omp_atk_fallback, omp_atv_null_fb), &
-            omp_alloctrait (omp_atk_pool_size, 8192)]
-  a = omp_init_allocator (omp_default_mem_space, 3, traits)
+            omp_alloctrait (omp_atk_fallback, omp_atv_null_fb)]
+  a = omp_init_allocator (omp_default_mem_space, 2, traits)
   if (a == omp_null_allocator) stop 1
 
   call omp_set_default_allocator (omp_default_mem_alloc);
-- 
2.25.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] [gfortran] Add support for allocate clause (OpenMP 5.0).
  2022-02-05 19:09                           ` Hafiz Abid Qadeer
@ 2022-02-16 10:29                             ` Hafiz Abid Qadeer
  0 siblings, 0 replies; 21+ messages in thread
From: Hafiz Abid Qadeer @ 2022-02-16 10:29 UTC (permalink / raw)
  To: Thomas Schwinge, Tobias Burnus, Jakub Jelinek; +Cc: gcc-patches, fortran

On 05/02/2022 19:09, Hafiz Abid Qadeer wrote:
> On 04/02/2022 11:25, Hafiz Abid Qadeer wrote:
>> On 04/02/2022 09:46, Thomas Schwinge wrote:
>>
>>>
>>> Abid, are you going to address these?  I think it does make sense if the
>>> C/C++ and Fortran test cases match as much as feasible.
>>>
>> Sure. I will do that.
> 
> The attached patch address those issues apart from removing pool_size trait.

Is this change ok to commit?

Thanks,
-- 
Hafiz Abid Qadeer


^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2022-02-16 10:29 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20211022130502.2211568-1-abidh@codesourcery.com>
2021-10-22 13:28 ` [PATCH] [gfortran] Add support for allocate clause (OpenMP 5.0) Tobias Burnus
     [not found] ` <20211102162714.GF304296@tucnak>
     [not found]   ` <e3c9083c-0906-699a-b437-84a49ece33b8@mentor.com>
     [not found]     ` <20211220200650.GN2646553@tucnak>
     [not found]       ` <fddcdfcf-3fab-1674-722e-2756a1d6aef8@mentor.com>
2022-01-14  9:10         ` Thomas Schwinge
2022-01-14 11:45           ` Tobias Burnus
2022-01-14 11:55             ` Jakub Jelinek
2022-01-14 12:20               ` Tobias Burnus
2022-01-17 14:01                 ` Hafiz Abid Qadeer
2022-01-21 17:15         ` Thomas Schwinge
2022-01-21 17:43           ` Tobias Burnus
2022-01-24  8:45             ` Tobias Burnus
2022-01-24 12:54               ` Hafiz Abid Qadeer
2022-01-25  9:19                 ` Thomas Schwinge
2022-01-25 10:32                   ` Tobias Burnus
2022-01-31 19:13                     ` Hafiz Abid Qadeer
2022-02-04  9:46                       ` Thomas Schwinge
2022-02-04 11:25                         ` Hafiz Abid Qadeer
2022-02-05 19:09                           ` Hafiz Abid Qadeer
2022-02-16 10:29                             ` Hafiz Abid Qadeer
2022-02-04  9:37               ` Thomas Schwinge
2022-02-04 13:57                 ` [committed] libgomp.fortran/allocate-1.f90: Minor cleanup (was: Re: [PATCH] [gfortran] Add support for allocate clause (OpenMP 5.0).) Tobias Burnus
2022-02-04 15:33                   ` Thomas Schwinge
2022-02-04 16:34                     ` Tobias Burnus

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).