[FFmpeg-devel] [PATCH 2/2] Use gcc/clang builtins for av_sat_(add|sub)_64_c if available.

[Martin Storsjö]

On Wed, 20 May 2020, James Almer wrote:

> On 5/20/2020 5:17 AM, Michael Niedermayer wrote:
>> On Mon, May 18, 2020 at 03:24:59PM -0700, Dale Curtis wrote:
>>> On Mon, May 18, 2020 at 3:24 PM Dale Curtis <dalecurtis at chromium.org> wrote:
>>>
>>>> On Mon, May 18, 2020 at 2:22 PM Michael Niedermayer <michael at niedermayer.cc>
>>>> wrote:
>>>>
>>>>>
>>>>>> 38cfdcfc9d4fa1c1239dc01a766e369b20a0232a  sat_math_builtin_v5.patch
>>>>>
>>>>
>>>> Latest upload is sat_math_builtin_v6.patch -- is that not showing up for
>>>> you? I just tested and it applies cleanly to trunk.
>>>>
>>>
>>> Ahh, crap that one got eaten because I used the wrong e-mail address to
>>> send it. Here's the latest one again from the right address this time.
>>>
>>> - dale
>>
>>>  common.h |   10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>> e153e8a9f60d075c1afcc58b2a5c18786d5965e5  sat_math_builtin_v6.patch
>>> From e3e69165c0a90302321e5fe3f05625235334cf57 Mon Sep 17 00:00:00 2001
>>> From: Dale Curtis <dalecurtis at chromium.org>
>>> Date: Fri, 1 May 2020 10:20:43 -0700
>>> Subject: [PATCH 2/2] Use gcc/clang builtins for av_sat_(add|sub)_64_c if
>>>  available.
>>>
>>> Signed-off-by: Dale Curtis <dalecurtis at chromium.org>
>>> ---
>>>  libavutil/common.h | 10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/libavutil/common.h b/libavutil/common.h
>>> index 7a774fc448..451d5d7383 100644
>>> --- a/libavutil/common.h
>>> +++ b/libavutil/common.h
>>> @@ -299,11 +299,16 @@ static av_always_inline int av_sat_dsub32_c(int a, int b)
>>>   * @return sum with signed saturation
>>>   */
>>>  static av_always_inline int64_t av_sat_add64_c(int64_t a, int64_t b) {
>>> +#if (!defined(__INTEL_COMPILER) && AV_GCC_VERSION_AT_LEAST(5,1)) || (defined(__clang__) && __has_builtin(__builtin_add_overflow))
>> 
>> CC	libavdevice/alldevices.o
>> In file included from ./libavutil/avutil.h:296:0,
>>                  from ./libavutil/log.h:25,
>>                  from ./libavutil/thread.h:34,
>>                  from libavdevice/alldevices.c:22:
>> ./libavutil/common.h: In function ‘av_sat_add64_c’:
>> ./libavutil/common.h:302:105: error: missing binary operator before token "("
>>  #if (!defined(__INTEL_COMPILER) && AV_GCC_VERSION_AT_LEAST(5,1)) || (defined(__clang__) && __has_builtin(__builtin_add_overflow))
>>                                                                                                          ^
>> In file included from ./libavutil/avutil.h:296:0,
>>                  from ./libavutil/log.h:25,
>>                  from ./libavutil/thread.h:34,
>>                  from libavdevice/alldevices.c:22:
>> ./libavutil/common.h: In function ‘av_sat_sub64_c’:
>> ./libavutil/common.h:322:105: error: missing binary operator before token "("
>>  #if (!defined(__INTEL_COMPILER) && AV_GCC_VERSION_AT_LEAST(5,1)) || (defined(__clang__) && __has_builtin(__builtin_sub_overflow))
>>                                                                                                          ^
>> ffbuild/common.mak:59: recipe for target 'libavdevice/alldevices.o' failed
>> make: *** [libavdevice/alldevices.o] Error 1
>
> __has_builtin() would have to be defined as 0 if not already defined by
> compiler headers, but I'm not comfortable at all doing it on a public
> header, so maybe this will have to be implemented within the arch
> folders in some way, even if it's arch agnostic.

Would it work better, if the conditions were nested on separate lines, 
e.g. instead of

#if SOMETHING && __has_builtin()

doing

#if SOMETHING
#if __has_builtin()


// Martin