[Mesa-dev] [PATCH] gallium/util: Define ffsll on OpenBSD.

Sat Mar 21 10:12:27 PDT 2015

On Sat, Mar 21, 2015 at 10:10 AM, Emil Velikov <emil.l.velikov at 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 <jsg at 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 <jsg at 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 <mattst88 at gmail.com>
>>>>>>>>>> Date:   Fri Feb 20 18:46:43 2015 -0800
>>>>>>>>>>
>>>>>>>>>>      gallium/util: Use HAVE___BUILTIN_* macros.
>>>>>>>>>>
>>>>>>>>>>      Reviewed-by: Eric Anholt <eric at anholt.net>
>>>>>>>>>>      Reviewed-by: Jose Fonseca <jfonseca at 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 <jsg at 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 <jsg at 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.velikov at 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.