[Bug libgomp/105358] New: [12 Regression] scan* fails on targets without aligned memory allocators.

[Bug libgomp/105358] New: [12 Regression] scan* fails on targets without aligned memory allocators.

[iains at gcc dot gnu.org]

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

            Bug ID: 105358
           Summary: [12 Regression] scan* fails on targets without aligned
                    memory allocators.
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libgomp
          Assignee: unassigned at gcc dot gnu.org
          Reporter: iains at gcc dot gnu.org
                CC: jakub at gcc dot gnu.org
  Target Milestone: ---

r12-4529-gc7abdf46fb7ac9a0c37 introduces a change to make use of efficient
allocators where they are available.

On i686/powerpc-darwin9 there are no such allocators (not even posix_memalign)
and so the GOMP_HAVE_EFFICIENT_ALIGNED_ALLOC flags is false.

the following change:

 struct gomp_work_share
 {
   /* This member records the SCHEDULE clause to be used for this construct.
@@ -324,7 +348,12 @@ struct gomp_work_share
      are in a different cache line.  */

   /* This lock protects the update of the following members.  */
+#ifdef GOMP_HAVE_EFFICIENT_ALIGNED_ALLOC
   gomp_mutex_t lock __attribute__((aligned (64)));
+#else
+  char pad[64 - offsetof (struct gomp_work_share_1st_cacheline, pad)];
+  gomp_mutex_t lock;
+#endif

causes struct gomp_work_share to become less aligned than 'long long' on the
32b host, which leads to wrong code in work.c which manipulates offsets with
__alignof__(long long).

So the underlying issue is that the target does guarantee malloc aligned to a
cache line [64 bytes on x86] (but only to the size of the largest vector
supported by the target [16 bytes]).

I'm not clear what the change above is implementing (since i'm not familiar
with how this interacts with the rest of the library) so I'm probably missing
some subtlety.

... but ISTM that we could omit that change, and the structure would always
claim 64 byte alignment (with the lock correctly at offset of 64 bytes).

Of course, on these older targets, malloc only returns something guaranteed to
be 16 byte aligned - but that *is* sufficient that any use of vector operations
to manipulate the content should see suitable alignment.

We could also just force the gomp_work_share struct to be aligned to
__alignof__(long long).
I guess we also have __BIGGEST_ALIGNMENT__ which would at least mean we could
align it suitably for the target default.

At present, I'm not proposing any patch - since I do not understand the
subtleties of how these objects interact with the library.

====

The reason that the fail is not seen on later versions (e.g. i686-darwin17) is
because posix_memalign () was introduced in Darwin10.

[Bug libgomp/105358] [12 Regression] scan* fails on targets without aligned memory allocators.

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |12.0

[Bug libgomp/105358] [12 Regression] scan* fails on targets without aligned memory allocators.

[jakub at gcc dot gnu.org]

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

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
That change was completely intentional, to fix Solaris/x86 etc.
If there is not an efficient aligned alloc, we don't want struct
gomp_work_share to require 64-byte alignment (because normal allocator can't
handle that and the emulated allocator is too costly) and just want to make
sure the two parts are 64-bytes appart.
The structure contains long long members, but sure, on ia32 long long has
smaller alignment in structures than __alignof__(long long).
Now, not really sure why does this matter if struct gomp_workshare is malloced,
the whole struct should get aligned to whatever malloc guarantees and that
better should include long long alignment.
Is this about
  struct gomp_work_share work_shares[8];
in struct gomp_team then?  We could for the #ifndef
GOMP_USE_ALIGNED_WORK_SHARES
case perhaps add __attribute__((aligned (MAX (__alignof__ (struct
gomp_work_share), __alignof__(long long)))) or so?

[Bug libgomp/105358] [12 Regression] scan* fails on targets without aligned memory allocators.

[jakub at gcc dot gnu.org]

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

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Does
--- libgomp/libgomp.h.jj        2022-01-11 23:11:23.890269075 +0100
+++ libgomp/libgomp.h   2022-04-25 11:20:09.744103064 +0200
@@ -717,6 +717,13 @@ struct gomp_team
   /* This barrier is used for most synchronization of the team.  */
   gomp_barrier_t barrier;

+#ifndef GOMP_USE_ALIGNED_WORK_SHARES
+  /* If struct gomp_work_share isn't 64-byte aligned, ensure it is
+     at least 8-byte aligned because __alignof__ (long long) is used
+     for the inline_ordered_team_ids handling.  */
+  struct { } pad __attribute__((aligned (__alignof__ (long long))));
+#endif
+
   /* Initial work shares, to avoid allocating any gomp_work_share
      structs in the common case.  */
   struct gomp_work_share work_shares[8];
fix it?

[Bug libgomp/105358] [12 Regression] scan* fails on targets without aligned memory allocators.

[iains at gcc dot gnu.org]

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

--- Comment #3 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #2)
> Does
> --- libgomp/libgomp.h.jj	2022-01-11 23:11:23.890269075 +0100
> +++ libgomp/libgomp.h	2022-04-25 11:20:09.744103064 +0200
> @@ -717,6 +717,13 @@ struct gomp_team
>    /* This barrier is used for most synchronization of the team.  */
>    gomp_barrier_t barrier;
>  
> +#ifndef GOMP_USE_ALIGNED_WORK_SHARES
> +  /* If struct gomp_work_share isn't 64-byte aligned, ensure it is
> +     at least 8-byte aligned because __alignof__ (long long) is used
> +     for the inline_ordered_team_ids handling.  */
> +  struct { } pad __attribute__((aligned (__alignof__ (long long))));
> +#endif
> +
>    /* Initial work shares, to avoid allocating any gomp_work_share
>       structs in the common case.  */
>    struct gomp_work_share work_shares[8];
> fix it?

unfortunately, no presumably the size of gomp_work_share is a multiple
of 4 bytes, whatwe need is to make the align of gomp_work_share at least
'long long'.

[Bug libgomp/105358] [12 Regression] scan* fails on targets without aligned memory allocators.

[jakub at gcc dot gnu.org]

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

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Still trying to understand where the problem is.
In work.c (gomp_init_work_share), there are 3 cases, one is ordered == 0,
another one ordered == 1, another one bigger numbers.  The first one doesn't
care,
ordered_team_ids is initialized to inline_ordered_team_ids just to make sure
we don't try to team_free it, isn't really used.  For ordered == 1 case, we
only use the trailing array as array of unsigned and most likely in the darwin9
case not at all, because I expect INLINE_ORDERED_TEAM_IDS_SIZE to be 0 or 4 or
something that small.
For the ordered > 1 case (used when some memory needs to be allocated after the
ordered_teams_ids array and that one should be possibly 8-byte aligned) it adds
extra __alignof__ (long long) - 1 to the size but undoes that if nthreads is
even and inline_ordered_team_ids is at position divisible by 8.

So, guess some question, can you e.g. from dumping DWARF on work.o find out:
sizeof (struct gomp_init_work_share)
offsetof (struct gomp_init_work_share, inline_ordered_team_ids)
?

[Bug libgomp/105358] [12 Regression] scan* fails on targets without aligned memory allocators.

[jakub at gcc dot gnu.org]

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

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Or is the problem mainly in code like
#define INLINE_ORDERED_TEAM_IDS_OFF \
  ((offsetof (struct gomp_work_share, inline_ordered_team_ids)          \
    + __alignof__ (long long) - 1) & ~(__alignof__ (long long) - 1))
          if (size > (sizeof (struct gomp_work_share)
                      - INLINE_ORDERED_TEAM_IDS_OFF))
where if struct gomp_work_share's size equal to inline_ordered_team_ids
isn't a multiple of 8 sizeof (struct gomp_work_share) -
INLINE_ORDERED_TEAM_IDS_OFF wraps around?

[Bug libgomp/105358] [12 Regression] scan* fails on targets without aligned memory allocators.

[iains at gcc dot gnu.org]

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

--- Comment #6 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #4)
> Still trying to understand where the problem is.
> In work.c (gomp_init_work_share), there are 3 cases, one is ordered == 0,
> another one ordered == 1, another one bigger numbers.  The first one doesn't
> care,
> ordered_team_ids is initialized to inline_ordered_team_ids just to make sure
> we don't try to team_free it, isn't really used.  For ordered == 1 case, we
> only use the trailing array as array of unsigned and most likely in the
> darwin9 case not at all, because I expect INLINE_ORDERED_TEAM_IDS_SIZE to be
> 0 or 4 or something that small.
> For the ordered > 1 case (used when some memory needs to be allocated after
> the
> ordered_teams_ids array and that one should be possibly 8-byte aligned) it
> adds extra __alignof__ (long long) - 1 to the size but undoes that if
> nthreads is even and inline_ordered_team_ids is at position divisible by 8.
> 
> So, guess some question, can you e.g. from dumping DWARF on work.o find out:
> sizeof (struct gomp_init_work_share)
> offsetof (struct gomp_init_work_share, inline_ordered_team_ids)
> ?

(sorry for being slow here, meetings...)

so 0xac = 172 .. and therefore as you expected INLINE_ORDERED_TEAM_IDS_SIZE = 0 
(that's on a 8 core / 16 thread machine)

0x00000dc5:     TAG_structure_type [22] *
                 AT_name( "gomp_work_share" )
                 AT_byte_size( 0xac )
                 AT_decl_file( "/src-local/gcc-master/libgomp/libgomp.h" )
                 AT_decl_line( 288 )
                 AT_decl_column( 0x08 )
                 AT_sibling( {0x00000f0a} )


0x00000ee4:         TAG_member [23]  
                     AT_name( "inline_ordered_team_ids" )
                     AT_decl_file( "/src-local/gcc-master/libgomp/libgomp.h" )
                     AT_decl_line( 395 )
                     AT_decl_column( 0x0c )
                     AT_type( {0x00001051} ( unsigned int[] ) )
                     AT_data_member_location( +172 )

[Bug libgomp/105358] [12 Regression] scan* fails on targets without aligned memory allocators.

[iains at gcc dot gnu.org]

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

--- Comment #7 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #5)
> Or is the problem mainly in code like
> #define INLINE_ORDERED_TEAM_IDS_OFF \
>   ((offsetof (struct gomp_work_share, inline_ordered_team_ids)          \
>     + __alignof__ (long long) - 1) & ~(__alignof__ (long long) - 1))
>           if (size > (sizeof (struct gomp_work_share)
>                       - INLINE_ORDERED_TEAM_IDS_OFF))
> where if struct gomp_work_share's size equal to inline_ordered_team_ids
> isn't a multiple of 8 sizeof (struct gomp_work_share) -
> INLINE_ORDERED_TEAM_IDS_OFF wraps around?

right - I suspect that could be the case because sizeof (struct
gomp_work_share) is not a multiple of 8.

[Bug libgomp/105358] [12 Regression] scan* fails on targets without aligned memory allocators.

[jakub at gcc dot gnu.org]

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

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 52865
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52865&action=edit
gcc12-pr105358.patch

So what about this?  All the newly added comparisons should fold into true or
false at compile time.

[Bug libgomp/105358] [12 Regression] scan* fails on targets without aligned memory allocators.

[iains at gcc dot gnu.org]

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

--- Comment #9 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #8)
> Created attachment 52865 [details]
> gcc12-pr105358.patch
> 
> So what about this?  All the newly added comparisons should fold into true
> or false at compile time.

Thanks, this works for me (tested two 32b hosts and one x86_64 one).

[Bug libgomp/105358] [12 Regression] scan* fails on targets without aligned memory allocators.

[cvs-commit at gcc dot gnu.org]

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

--- Comment #10 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:57a957cb71c004de80b0fd30c8db3cc67576e0ce

commit r12-8259-g57a957cb71c004de80b0fd30c8db3cc67576e0ce
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Tue Apr 26 08:57:17 2022 +0200

    libgomp: Fix up two non-GOMP_USE_ALIGNED_WORK_SHARES related issues
[PR105358]

    Last fall I've changed struct gomp_work_share, so that it doesn't have
    __attribute__((aligned (64))) lock member in the middle unless the target
has
    non-emulated aligned allocator, otherwise it just makes sure the first and
    second halves are 64 bytes appart for cache line reasons, but doesn't make
    the struct 64-byte aligned itself and so we can use normal allocators for
it.

    When the struct isn't 64-byte aligned, the amount of tail padding
significantly
    decreases, to 0 or 4 bytes or so.  The library uses that tail padding when
    the ordered_teams_ids array (array of uints) and/or the memory for
lastprivate
    conditional temporaries (the latter wants to guarantee long long
alignment).
    The problem with it on ia32 darwin9 is that while the struct contains
    long long members, long long is just 4 byte aligned while __alignof__(long
long)
    is 8.  That causes problems in gomp_init_work_share, where we currently
rely on
    if offsetof (struct gomp_work_share, inline_ordered_team_ids) is long long
    aligned, then that tail array will be aligned at runtime and so no extra
    memory for dynamic realignment will be needed (that is false when the whole
    struct doesn't have long long alignment).  And also in the remaining hunks
    causes another problem, where we compute INLINE_ORDERED_TEAM_IDS_OFF
    as the above offsetof aligned up to long long boundary and subtract
    sizeof (struct gomp_work_share) and INLINE_ORDERED_TEAM_IDS_OFF.
    When unlucky, the former isn't multiple of 8 and the latter is 4 bigger
    than that and as the subtraction is done in size_t, we end up with (size_t)
-4,
    so the comparison doesn't really work.

    The fixes add additional conditions to make it work properly, but all of
them
    should be evaluated at compile time when optimizing and so shouldn't slow
    anything.

    2022-04-26  Jakub Jelinek  <jakub@redhat.com>

            PR libgomp/105358
            * work.c (gomp_init_work_share): Don't mask of adjustment for
            dynamic long long realignment if struct gomp_work_share has smaller
            alignof than long long.
            * loop.c (GOMP_loop_start): Don't use inline_ordered_team_ids if
            struct gomp_work_share has smaller alignof than long long or if
            sizeof (struct gomp_work_share) is smaller than
            INLINE_ORDERED_TEAM_IDS_OFF.
            * loop_ull.c (GOMP_loop_ull_start): Likewise.
            * sections.c (GOMP_sections2_start): Likewise.

[Bug libgomp/105358] [12 Regression] scan* fails on targets without aligned memory allocators.

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed.