Re: [PATCH 4/6] Add macros for increasing data alignment

["Måns Rullgård" <mans@mansr.com>]

Eric Wong <normalperson@yhbt.net> writes:

> Måns Rullgård <mans@mansr.com> wrote:
>> That CPU doesn't have AVX so 16-byte alignment is enough, and plain
>> malloc usually provides that.  It obviously doesn't hurt to add support
>> for memalign as well even though it is considered obsolete.  If you do
>> that, you should also take care of #including malloc.h.
>
> Yes, added memalign(3) and posix_memalign(3) fallbacks.
> I also went ahead and stole a bit from Ruby to provide a
> fallback when none of the 3 functions exist.
>
> Will still need to deal with systems without LSX_ALIGN...

Almost all compilers support either the GNU or the MSVC syntax.  The few
that don't probably don't support AVX anyhow.  Mostly it's compilers for
obscure DSPs.

> Anyways, pushed the following to git://bogomips.org/sox ew/align
>
> -----------------8<------------------
> Subject: [PATCH] always support aligned heap allocation
>
> The new sdm effect will not work correctly on AVX systems without
> 32-byte alignment; so we need to support older systems without
> C11 aligned_alloc.
>
> The fallback emulation is based on code found in gc.c in Ruby 2.0+
> (BSD-licensed)
> ---
>  configure.ac | 11 ++++++++++-
>  src/util.h   | 31 +++++++++++++++++++++++++++++--
>  2 files changed, 39 insertions(+), 3 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 8570638..017a9dd 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -207,7 +207,16 @@ AC_HEADER_STDC
>  AC_CHECK_HEADERS(fcntl.h unistd.h byteswap.h sys/stat.h sys/time.h sys/timeb.h sys/types.h sys/utsname.h termios.h glob.h fenv.h)
>
>  dnl Checks for library functions.
> -AC_CHECK_FUNCS(strcasecmp strdup popen vsnprintf gettimeofday mkstemp fmemopen aligned_alloc)
> +AC_CHECK_FUNCS(strcasecmp strdup popen vsnprintf gettimeofday mkstemp fmemopen)
> +
> +dnl aligned alloc required for sdm using AVX (32-byte) or SSE2 (16-byte)
> +AC_CHECK_FUNCS(aligned_alloc)
> +AS_IF([test "x$ac_cv_func_aligned_alloc" != xyes], [
> +	AC_CHECK_FUNCS([memalign])
> +	AS_IF([test "x$ac_cv_func_memalign" != xyes], [
> +		AC_CHECK_FUNCS([posix_memalign])
> +	])
> +])

Why don't you just add (posix_)memalign to the existing list of
functions?  It's not an error if some of them don't exist, and you're
checking the resulting HAVE_ macros in the same order anyway.

>  dnl Check if math library is needed.
>  AC_SEARCH_LIBS([pow], [m])
> diff --git a/src/util.h b/src/util.h
> index b5cc9b8..3a9686d 100644
> --- a/src/util.h
> +++ b/src/util.h
> @@ -196,12 +196,39 @@
>
>  #ifdef HAVE_ALIGNED_ALLOC
>    #define aligned_free(p) free(p)
> +#elif defined(HAVE_MEMALIGN)
> +  #include <malloc.h>
> +  #define aligned_alloc(a, s) memalign(a, s)
> +  #define aligned_free(p) free(p)
> +#elif defined(HAVE_POSIX_MEMALIGN)
> +  #include <errno.h>
> +static inline void *sox_aligned_alloc_pm(size_t align, size_t size)
> +{
> +  void *ptr;
> +  int err = posix_memalign(&ptr, align, size);
> +
> +  if (!err) return ptr;
> +  errno = err;
> +  return 0;
> +}
> +  #define aligned_alloc(a, s) sox_aligned_alloc_pm(a, s)
> +  #define aligned_free(p) free(p)
>  #elif defined _MSC_VER
>    #define aligned_alloc(a, s) _aligned_malloc(s, a)
>    #define aligned_free(p) _aligned_free(p)
>  #else
> -  #define aligned_alloc(a, s) malloc(s)
> -  #define aligned_free(p) free(p)
> +static inline void *sox_aligned_alloc_m(size_t align, size_t size)
> +{
> +  void *res = malloc(align + size + sizeof(void *));
> +  char *aligned = (char *)res + align + sizeof(void *);
> +
> +  aligned -= ((size_t)aligned & (align - 1));

Use uintptr_t rather than size_t there.  Although they are usually the
same underlying type, there is no such guarantee, especially for systems
bizarre enough not to have an aligned allocation function.

> +  ((void **)aligned)[-1] = res;
> +  return (void *)aligned;
> +}
> +
> +  #define aligned_alloc(a, s) sox_aligned_alloc_m(a, s)
> +  #define aligned_free(p) free(((void**)p)[-1]);
>  #endif
>
>  /*------------------------------- Maths stuff --------------------------------*/
> -- 
> EW

-- 
Måns Rullgård

------------------------------------------------------------------------------