On Mon, Mar 16, 2015 at 08:37:28PM +0000, Emil Velikov wrote: > On 26/02/15 13:49, Jose Fonseca wrote: > > On 26/02/15 13:42, Jose Fonseca wrote: > >> On 26/02/15 03:55, Jonathan Gray wrote: > >>> On Wed, Feb 25, 2015 at 07:09:26PM -0800, Matt Turner wrote: > >>>> On Wed, Feb 25, 2015 at 7:03 PM, Jonathan Gray <j...@jsg.id.au> wrote: > >>>>> On Wed, Feb 25, 2015 at 06:53:14PM -0800, Matt Turner wrote: > >>>>>> On Wed, Feb 25, 2015 at 5:37 PM, Jonathan Gray <j...@jsg.id.au> wrote: > >>>>>>> If it isn't going to be configure checks could someone merge the > >>>>>>> original patch in this thread? > >>>>>> > >>>>>> I committed > >>>>>> > >>>>>> commit 3492e88090d2d0c0bfbc934963b8772b45fc8880 > >>>>>> Author: Matt Turner <matts...@gmail.com> > >>>>>> Date: Fri Feb 20 18:46:43 2015 -0800 > >>>>>> > >>>>>> gallium/util: Use HAVE___BUILTIN_* macros. > >>>>>> > >>>>>> Reviewed-by: Eric Anholt <e...@anholt.net> > >>>>>> Reviewed-by: Jose Fonseca <jfons...@vmware.com> > >>>>>> > >>>>>> which switched over a bunch of preprocessor checks around __builtin* > >>>>>> calls to use the macros defined by autotools. > >>>>>> > >>>>>> So I think cleaning it up to use __builtin_ffs* first #ifdef > >>>>>> HAVE___BUILTIN_* can go forward now. > >>>>> > >>>>> Yes but there is no HAVE_FFSLL for constructs like > >>>>> > >>>>> #if !defined(HAVE_FFSLL) && defined(HAVE___BUILTIN_FFSLL) > >>>>> > >>>>> or is it ok to always use the builtin? > >>>> > >>>> I think the question is whether it's okay to always use the builtin if > >>>> it's available (as opposed to libc functions). I think the answer to > >>>> that is yes. > >>> > >>> So in that case how about the following? Or is it going to break > >>> the android scons build? > >>> > >>> From cba39ba72115e57d262cb4b099c4e72106f01812 Mon Sep 17 00:00:00 2001 > >>> From: Jonathan Gray <j...@jsg.id.au> > >>> Date: Thu, 26 Feb 2015 14:46:45 +1100 > >>> Subject: [PATCH] gallium/util: use ffs* builtins if available > >>> > >>> Required to build on OpenBSD which doesn't have ffsll in libc. > >>> > >>> Signed-off-by: Jonathan Gray <j...@jsg.id.au> > >>> --- > >>> src/gallium/auxiliary/util/u_math.h | 11 ++++++++--- > >>> 1 file changed, 8 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/src/gallium/auxiliary/util/u_math.h > >>> b/src/gallium/auxiliary/util/u_math.h > >>> index b4a65e4..5bc9b97 100644 > >>> --- a/src/gallium/auxiliary/util/u_math.h > >>> +++ b/src/gallium/auxiliary/util/u_math.h > >>> @@ -384,9 +384,6 @@ unsigned ffs( unsigned u ) > >>> > >>> return i; > >>> } > >>> -#elif defined(__MINGW32__) || defined(PIPE_OS_ANDROID) > >>> -#define ffs __builtin_ffs > >>> -#define ffsll __builtin_ffsll > >> > >> Scons does define HAVE___BUILTIN_FFS for mingw. > >> > >> However `git grep '\<ffs\>` shows ffs is used directly in many other > >> places. So I suspect this change will break them. > >> > >>> #endif > >>> > >>> #endif /* FFS_DEFINED */ > >>> @@ -435,7 +432,11 @@ util_last_bit_signed(int i) > >>> static INLINE int > >>> u_bit_scan(unsigned *mask) > >>> { > >>> +#if defined(HAVE___BUILTIN_FFS) > >>> + int i = __builtin_ffs(*mask) - 1; > >>> +#else > >>> int i = ffs(*mask) - 1; > >>> +#endif > >>> *mask &= ~(1 << i); > >>> return i; > >>> } > >>> @@ -444,7 +445,11 @@ u_bit_scan(unsigned *mask) > >>> static INLINE int > >>> u_bit_scan64(uint64_t *mask) > >>> { > >>> +#if defined(HAVE___BUILTIN_FFSLL) > >>> + int i = __builtin_ffsll(*mask) - 1; > >>> +#else > >>> int i = ffsll(*mask) - 1; > >>> +#endif > >>> *mask &= ~(1llu << i); > >>> return i; > >>> } > >>> > >> > >> > >> I think the right thing long term is to provide ffs and ffsll in > >> c99_compat.h or c99_math.h for all platforms. And let the rest of the > >> code just always assume it's available somehow. > >> > >> > >> Otherwise, let's just '#define ffs __builtin_ffs' on OpenBSD too. > > > > In other words, the original patch on this thread > > > > http://lists.freedesktop.org/archives/mesa-dev/2015-February/076071.html > > > > is the only patch I've seen so far that doesn't break Mingw. > > > > If you rather use HAVE___BUILTIN_FFSLL, then just do > > > > diff --git a/src/gallium/auxiliary/util/u_math.h > > b/src/gallium/auxiliary/util/u_math.h > > index 959f76e..d372cfd 100644 > > --- a/src/gallium/auxiliary/util/u_math.h > > +++ b/src/gallium/auxiliary/util/u_math.h > > @@ -384,7 +384,7 @@ unsigned ffs( unsigned u ) > > > > return i; > > } > > -#elif defined(__MINGW32__) || defined(PIPE_OS_ANDROID) > > +#elif defined(__MINGW32__) || defined(PIPE_OS_ANDROID) || > > defined(HAVE___BUILTIN_FFSLL) > > #define ffs __builtin_ffs > > #define ffsll __builtin_ffsll > > #endif > > > Jonathan > > Seems like this has ended up a longer discussion that anticipated :\ > Can you please confirm if the above works for you ? > > Thanks > Emil
It looks like that diff was mangled by the mail client and doesn't have the newline escaped. It also assumes a ffsll builtin implies a ffs builtin is present. So how about the following instead: diff --git a/src/gallium/auxiliary/util/u_math.h b/src/gallium/auxiliary/util/u_math.h index 8f62cac..89c63d7 100644 --- a/src/gallium/auxiliary/util/u_math.h +++ b/src/gallium/auxiliary/util/u_math.h @@ -383,14 +383,28 @@ unsigned ffs( unsigned u ) return i; } -#elif defined(__MINGW32__) || defined(PIPE_OS_ANDROID) +#elif defined(__MINGW32__) || defined(PIPE_OS_ANDROID) || \ + defined(HAVE___BUILTIN_FFS) #define ffs __builtin_ffs -#define ffsll __builtin_ffsll #endif #endif /* FFS_DEFINED */ /** + * Find first bit set in long long. Least significant bit is 1. + * Return 0 if no bits set. + */ +#ifndef FFSLL_DEFINED +#define FFSLL_DEFINED 1 + +#if defined(__MINGW32__) || defined(PIPE_OS_ANDROID) || \ + defined(HAVE___BUILTIN_FFSLL) +#define ffsll __builtin_ffsll +#endif + +#endif /* FFSLL_DEFINED */ + +/** * Find last bit set in a word. The least significant bit is 1. * Return 0 if no bits are set. */ _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev