On 21/03/15 17:12, Matt Turner wrote:
> On Sat, Mar 21, 2015 at 10:10 AM, Emil Velikov <emil.l.veli...@gmail.com>
> wrote:
>> On 17/03/15 23:44, Emil Velikov wrote:
>>> 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>
>>>
>> Fwiw, I will be commiting this in the next few days unless there are any
>> objections.
>
> Seems fine as a little bandaid, but I think ffs was one of the reasons
> we have -Isrc/gallium/include all over the tree. I'd be great (and
> really easy) to move it to src/util.
>
I'm not 100% sure on the -Isrc/gallium/foo part as we already have
alternative ffs* implementations in src/mesa/main/imports.h. That aside
it's a small bandaid which will do the job until the next round of cleanups.
Cheers,
Emil
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
