Re: [PATCH] arm: Don't require a runtime check function for targets with unconditional NEON

On Tue, 11 Oct 2022, Cosmin Truta wrote:

> On Tue, Oct 11, 2022 at 5:50 PM Martin Storsjö <[email protected]> wrote:
>
>> >> On the other hand, I do agree that the configure script and the cmake
>> >> file should be consistent in their behavior.
>>
>> Sure. Unless the user specified anything, currently CMake sets
>> PNG_ARM_NEON to "check" (and thus defines PNG_ARM_NEON_CHECK_SUPPORTED),
>> while the configure script doesn't (but it should be easy to change
>> configure to do the same).
>
> Unless somebody else says otherwise, my response is that,
> historically, the cmake file has been changing its behavior in all
> kinds of ways, while the configure script was more stable. I'd rather
> change the CMake file to follow the configure script.
>
> On top of that, there's the issue of libpng users who use neither the
> configure script, nor the cmake file, but rather, take the source code
> and embed it into their own build systems. Very notable examples:
> Android AOSP and Chromium.
>
> My point here is that if the source code can configure itself without
> a necessary aid from the external configure or cmake scripts, that'd
> be the best.

That sounds reasaonable to me.

> For your testing, you can see how the makefiles are working:
>
> ./ci/ci_autotools.sh
> ./ci/ci_cmake.sh
> ./ci/ci_legacy.sh
>
>> If we want to avoid the inconsistency, either pngpriv.h will undef
>> PNG_ARM_NEON_CHECK_SUPPORTED when it realizes that it indeed doesn't need
>> any check - this is quite simple to achieve. (Essentially what my patch
>> does, but the undef can be moved closer to the "#define PNG_ARM_NEON_OPT
>> 2" to avoid inconsistencies over longer ranges.)
>
> I like this. It will work correctly, without requiring configure or
> cmake to work correctly.
>
> On top of that...
>
>> Or we can have the build
>> scripts try to compile a small file to check whether __ARM_NEON__ and/or
>> __ARM_NEON is defined, and default to "on" instead of "check". This is a
>> bit more of code (as the logic needs to be duplicated in both cmake and
>> configure).
>
> This is also something good to have, although -- again, in my personal
> opinion only -- is secondary, but still useful.

Yeah, I kinda rather have the logic in well contained source than 
duplicated in lots of different build systems.

> It's secondary, meaning that the source code itself should be able to
> do the autodetection. And it's still useful, meaning that an
> inconsistency between the user requesting the "check" option should be
> told, by the means of a warning, or even an error, that the "check" is
> being disabled.

Maybe - although for a warning, having pngpriv.h emit it would probably be 
quite spammy, as it would be visible in every single object file built.

> I wish I had more expertise on this, but I don't. These are just ideas
> off the top of my head.
>
> I wonder what's the best industry practice, or, at least, what is done
> in other projects that also implement this kind of optimization in
> this kind of configurable way.

Most such projects that I've seen don't allow the user to configure it to 
quite this level; they have a list of conditions for various 
architectures/OSes for enabling it, but I haven't really seen this level 
of configurability where the logic is in the hands of the user and 
configure script before. E.g. something like this:

https://code.videolan.org/videolan/dav1d/-/blob/99172b11470776177939c3d2bc366fe8d904eab7/src/arm/cpu.c#L83-99

// Martin

_______________________________________________
png-mng-implement mailing list
png-mng-implement-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/png-mng-implement