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

[James Almer]

On 5/1/2020 5:01 PM, Carl Eugen Hoyos wrote:
> Am Fr., 1. Mai 2020 um 21:58 Uhr schrieb James Almer <jamrial at gmail.com>:
>>
>> On 5/1/2020 4:46 PM, Carl Eugen Hoyos wrote:
>>> Am Fr., 1. Mai 2020 um 21:42 Uhr schrieb James Almer <jamrial at gmail.com>:
>>>>
>>>> On 5/1/2020 4:37 PM, Carl Eugen Hoyos wrote:
>>>>> Am Fr., 1. Mai 2020 um 21:34 Uhr schrieb James Almer <jamrial at gmail.com>:
>>>>>>
>>>>>> On 5/1/2020 4:27 PM, Carl Eugen Hoyos wrote:
>>>>>>> Am Fr., 1. Mai 2020 um 20:22 Uhr schrieb James Almer <jamrial at gmail.com>:
>>>>>>>>
>>>>>>>> On 5/1/2020 3:07 PM, Carl Eugen Hoyos wrote:
>>>>>>>>> Am Fr., 1. Mai 2020 um 19:24 Uhr schrieb Dale Curtis <dalecurtis at chromium.org>:
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Dale Curtis <dalecurtis at chromium.org>
>>>>>>>>>> ---
>>>>>>>>>>  libavutil/common.h | 10 ++++++++++
>>>>>>>>>>  1 file changed, 10 insertions(+)
>>>>>>>>>
>>>>>>>>> Imo, this is guaranteed to break x86 compilation with some unusual
>>>>>>>>> toolchain.
>>>>>>>>> I looked carefully at all occurrences of AV_GCC_VERSION_AT_LEAST()
>>>>>>>>> and I believe they are in fact different (not for x86 or combined with other
>>>>>>>>> checks).
>>>>>>>>
>>>>>>>> Can you elaborate on this? AV_GCC_VERSION_AT_LEAST() is a public macro
>>>>>>>> used in public headers to choose one or another path depending on if the
>>>>>>>> compiler is a recent enough GCC, or something else.
>>>>>>>
>>>>>>>> What toolchain could this break
>>>>>>>
>>>>>>> It definitely breaks icc compilation on Linux.
>>>>>>
>>>>>> So ICC on Linux defines __GNUC__ >= 5 yet doesn't support these builtins?
>>>>>
>>>>> No, but yes, this is the effect.
>>>>
>>>> So that means ICC is broken, defining support for features it doesn't
>>>> really has.
>>>> We can workaround that with a !defined(__INTEL_COMPILER) check, i guess,
>>>> but perhaps we may want to stop supporting broken compilers.
>>>
>>> Or we add the necessary two lines of configure code necessary to avoid
>>> the issue.
>>
>> No, a pre-processor check is more than enough. If we really care about
>> ICC, then Dale Curtis can add !defined(__INTEL_COMPILER) to the check,
>> like it's done in plenty other places for this exact same purpose.
> 
> No, it is not enough.

Unless you explain why, with details, I'll have to ignore you.
One-liners that don't argue anything are simply not helpful and a waste
of time.

> 
>>>>>>>> and why specifically x86?
>>>>>>>
>>>>>>> I mentioned x86 because afaics, all existing relevant uses of
>>>>>>> AV_GCC_VERSION_AT_LEAST() will not be triggered on x86.
>>>>>>
>>>>>> AV_GCC_VERSION_AT_LEAST is used in a lot of arch agnostic libavutil
>>>>>> headers, and in x86/intmath.h
>>>>>
>>>>> This is exactly what I meant above with "carefully".
>>>>
>>>> It would be nice if you could stop being so terse and explain what's on
>>>> your mind for once. You're not being clear at all, and i asked you to
>>>> elaborate, something you still haven't done.
>>>
>>> To make this crystal-clear:
>>> It would be nice if you could stop writing offending and annoying mails as
>>> you did today.
>>
>> How am i writing offending emails? I asked you to elaborate on something
>> and barely got any information in return, then asked you to be less
>> terse and actually elaborate on what you were trying to argue.
> 
> This seems untrue to me.

Yet you wrote another unhelpful one-liner with no information whatsoever
in this very email up there.

> 
>>> The intmath.h check is surrounded by an additional check that looks much
>>> stricter to me than the AV_GCC_VERSION_AT_LEAST() check there.
>>
>> The AV_GCC_VERSION_AT_LEAST check in x86/intmath.h is wrapped in a
>> __GNUC__ check and a __BMI2__ check. The former has no effect as far as
>> the macro is concerned since it's already a part of it, and the latter
>> is yet another GCC define, hence being checked after __GNUC__.
>> And being in the x86 folder, it's of course triggered on x86 targets.
>>
>> Meanwhile, all the AV_GCC_VERSION_AT_LEAST checks in attributes.h are
>> triggered for every single target, with considerations for clang and ICC
>> where required.
> 
> Which case in attributes.h can cause a compilation failure?
> I am curious because iirc I wrote some of them, I definitely tested them.
> 
> Carl Eugen
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>