[Bug target/56726] New: i386: MALLOC_ABI_ALIGNMENT is too small (usually)

[Bug target/56726] New: i386: MALLOC_ABI_ALIGNMENT is too small (usually)

[chip at pobox dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56726

             Bug #: 56726
           Summary: i386: MALLOC_ABI_ALIGNMENT is too small (usually)
    Classification: Unclassified
           Product: gcc
           Version: 4.9.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: chip@pobox.com


Observed malloc alignment for the i386 ABI is double POINTER_SIZE. 
BITS_PER_WORD, the current default, is usually too small.  (It's right only on
X32.)

Proposed patch:

--- gcc/config/i386/i386.h      (revision 197055)
+++ gcc/config/i386/i386.h      (working copy)
@@ -815,6 +815,14 @@
    x86_field_alignment (FIELD, COMPUTED)
 #endif

+/* The maximum alignment 'malloc' honors.
+
+   This value is taken from glibc documentation for memalign().  It may
+   be up to double the very conservative GCC default.  This should be safe,
+   since even the GCC 4.8 default of BIGGEST_ALIGNMENT usually worked.  */
+
+#define MALLOC_ABI_ALIGNMENT (POINTER_SIZE * 2)
+
 /* If defined, a C expression to compute the alignment given to a
    constant that is being placed in memory.  EXP is the constant
    and ALIGN is the alignment that the object would ordinarily have

[Bug target/56726] i386: MALLOC_ABI_ALIGNMENT is too small (usually)

[hjl.tools at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56726

H.J. Lu <hjl.tools at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hjl.tools at gmail dot com

--- Comment #1 from H.J. Lu <hjl.tools at gmail dot com> 2013-03-25 20:04:44 UTC ---
(In reply to comment #0)
> Observed malloc alignment for the i386 ABI is double POINTER_SIZE. 
> BITS_PER_WORD, the current default, is usually too small.  (It's right only on
> X32.)
> 
> Proposed patch:
> 
> --- gcc/config/i386/i386.h      (revision 197055)
> +++ gcc/config/i386/i386.h      (working copy)
> @@ -815,6 +815,14 @@
>     x86_field_alignment (FIELD, COMPUTED)
>  #endif
> 
> +/* The maximum alignment 'malloc' honors.
> +
> +   This value is taken from glibc documentation for memalign().  It may
> +   be up to double the very conservative GCC default.  This should be safe,
> +   since even the GCC 4.8 default of BIGGEST_ALIGNMENT usually worked.  */
> +
> +#define MALLOC_ABI_ALIGNMENT (POINTER_SIZE * 2)
> +

This isn't correct for x32, whose pointer size is 4 bytes with malloc
aligned to 16 bytes.

[Bug target/56726] i386: MALLOC_ABI_ALIGNMENT is too small (usually)

[chip at pobox dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56726

--- Comment #2 from Chip Salzenberg <chip at pobox dot com> 2013-03-25 21:35:19 UTC ---
I'm a bit skeptical of that.  Glibc malloc alignment is 2 * sizeof(void*), and
void* in X32 is 32 bits.  Unless X32 code uses the x86_64 libc, I am confused.

PS: Hi, HJ

[Bug target/56726] i386: MALLOC_ABI_ALIGNMENT is too small (usually)

[hjl.tools at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56726

--- Comment #3 from H.J. Lu <hjl.tools at gmail dot com> 2013-03-25 22:07:18 UTC ---
(In reply to comment #2)
> I'm a bit skeptical of that.  Glibc malloc alignment is 2 * sizeof(void*), and
> void* in X32 is 32 bits.  Unless X32 code uses the x86_64 libc, I am confused.
> 
> PS: Hi, HJ

Hi Chip,

The current glibc has

#ifndef MALLOC_ALIGNMENT
# if !SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_16)
/* This is the correct definition when there is no past ABI to constrain it.

   Among configurations with a past ABI constraint, it differs from
   2*SIZE_SZ only on powerpc32.  For the time being, changing this is
   causing more compatibility problems due to malloc_get_state and
   malloc_set_state than will returning blocks not adequately aligned for
   long double objects under -mlong-double-128.  */

#  define MALLOC_ALIGNMENT       (2 * SIZE_SZ < __alignof__ (long double) \
                                  ? __alignof__ (long double) : 2 * SIZE_SZ)
# else
#  define MALLOC_ALIGNMENT       (2 * SIZE_SZ)
# endif
#endif

[Bug target/56726] i386: MALLOC_ABI_ALIGNMENT is too small (usually)

[chip at pobox dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56726

--- Comment #4 from Chip Salzenberg <chip at pobox dot com> 2013-03-25 22:35:57 UTC ---
If I'm reading that correctly, it seems to agree with my patch.

It looks like MALLOC_ABI_ALIGNMENT of POINTER_SIZE*2 is always either correct
or smaller than necessary, but never too large.  If MALLOC_ABI_ALIGNMENT is
smaller than necessary then optimizations may be missed (depending on the
values).  But if it is too large then performance *will* suffer.  It might even
cause exceptions from unaligned accesses, but i386 is very forgiving, so it'll
just be slower for no apparent reason.

Perhaps the glibc version differences in malloc should be advertised with
__attribute__ on the malloc declarations.  Perhaps a new pragma or attribute is
required to do this 100% right.  But in the meantime I like POINTER_SIZE*2.

[Bug target/56726] i386: MALLOC_ABI_ALIGNMENT is too small (usually)

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56726

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |missed-optimization
             Target|                            |x86_64-*-*, i?86-*-*
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2013-03-26
     Ever Confirmed|0                           |1

--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> 2013-03-26 14:27:03 UTC ---
Confirmed.

[Bug target/56726] i386: MALLOC_ABI_ALIGNMENT is too small (usually)

[chip at pobox dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56726

--- Comment #6 from Chip Salzenberg <chip at pobox dot com> 2013-03-29 06:05:19 UTC ---
May I have this accepted?

[Bug target/56726] i386: MALLOC_ABI_ALIGNMENT is too small (usually)

[chip at pobox dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56726

--- Comment #7 from Chip Salzenberg <chip at pobox dot com> ---
Should this ticket have status CONFIRMED ? Also I suspect it's been fixed in
trunk...

[Bug target/56726] i386: MALLOC_ABI_ALIGNMENT is too small (usually)

[chip at pobox dot com]

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

--- Comment #8 from Chip Salzenberg <chip at pobox dot com> ---
Further research says that the alignment of a malloc(N) will be >= N if there
is a basic type that requires alignment N.  So we may be able to ramp this up
quite a bit.

[Bug target/56726] i386: MALLOC_ABI_ALIGNMENT is too small (usually)

[fweimer at redhat dot com]

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

Florian Weimer <fweimer at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |fweimer at redhat dot com

--- Comment #9 from Florian Weimer <fweimer at redhat dot com> ---
On x86-64, Both jemalloc and tcmalloc return 8 byte aligned pointers for sizes
of 8 and less.  It's not true that malloc guarantuees 16-byte alignment there. 
You cannot look at glibc malloc implementation details and infer ABI properties
from that.

[Bug target/56726] i386: MALLOC_ABI_ALIGNMENT is too small (usually)

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Then jemalloc and tcmalloc are just broken on x86_64.

[Bug target/56726] i386: MALLOC_ABI_ALIGNMENT is too small (usually)

[chip at pobox dot com]

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

--- Comment #11 from Chip Salzenberg <chip at pobox dot com> ---
Indeed, 16 is required by the ABI; see
http://www.x86-64.org/documentation/abi.pdf page 12.  Only the SIMD __m256 is
bigger than 16, and there seems no end to Intel's extensions to SIMD registers,
so holding at 16 seems like the Right Thing.

[Bug target/56726] i386: MALLOC_ABI_ALIGNMENT is too small (usually)

[fweimer at redhat dot com]

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

--- Comment #12 from Florian Weimer <fweimer at redhat dot com> ---
(In reply to Chip Salzenberg from comment #11)
> Indeed, 16 is required by the ABI; see
> http://www.x86-64.org/documentation/abi.pdf page 12.  Only the SIMD __m256
> is bigger than 16, and there seems no end to Intel's extensions to SIMD
> registers, so holding at 16 seems like the Right Thing.

Even for allocations smaller than 16 bytes?