On 17/03/15 01:25, Jonathan Gray wrote:
> 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.
> */
>
Looks ok to me. Afaict splitting out __builtin_ffs and __builtin_ffsll
is a nice idea, as one does have to imply the other. Haven't seen any
references behind the FFS_DEFINED guards, although I'd assume that is to
prevent clashing with the one in classic mesa.
Reviewed-by: Emil Velikov <emil.l.veli...@gmail.com>
Cheers,
Emil
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
