Re: [PATCH] [gfortran] Add support for allocate clause (OpenMP 5.0).

[Thomas Schwinge <thomas@codesourcery.com>]

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