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

--===============3382500824848788342==
Content-Type: multipart/alternative; boundary="0000000000004fd0ae05eac58be7"

--0000000000004fd0ae05eac58be7
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Oct 11, 2022 at 10:05 AM Martin Storsj=C3=B6 <[email protected]> wro=
te:


> In the cmake build (contrary to the configure script!),
> PNG_ARM_NEON_CHECK_SUPPORTED gets defined by default when building for an
> ARM target, unless the user requested something else.
>

That is the bug in *the cmake build*; as the comments point out cmake is
not building pnglibconf.h because neither I nor Glenn could work out how to
invoke awk from cmake.

In any case the "check" option is very, very deprecated and should not be
the default anywhere; API is fine.  arm/arm_init.c even complains loudly if
it is set on arm64s.


> The NEON runtime check is implemented for Linux, but not for other OSes,
> hitting #error "No support for run-time ARM Neon checking; use
> compile-time options" when building for iOS and Windows on armv7.
>

Yep.  It's deprecated.


>
> While the user could override this when configuring the build, I though i=
t
> could be avoided - on iOS and Windows on armv7, NEON is part of the
> platform baseline, so the compiler is free to use NEON anywhere in code
> generated from the C code, so no runtime check should be needed. The
> compiler signals this, that NEON code generation is enabled
> unconditionally, by defining __ARM_NEON__ (or __ARM_NEON).
>

Nope; you are fixing a cmake bug by breaking an API.  Sure, removing the
API is fine, but just breaking it because of a simple failure to update
CMakeLists.txt is clearly wrong.

Make the obvious changes aroung line 1040 of CMakeLists.txt!.  Or, better,
use the configure build which is way more up-to-date than the cmake stuff.
For sure the cmake build should not build a different default
configuration.   Here's the default:

1) PNG_ARM_NEON_OPT *should be unset by default.*  Otherwise multilib
builds are broken.
2) Neither PNG_ARM_NEON_*CHECK* nor *API *should be defined by default;
they should be options.  Both of them can be set at the same time IRC.
3) The settings should be reflected in the installed pnglibconf.h
4) Much better; cmake should behave like configure, take the same options
with the same defaults (not invent a new default configuration) and
implement these via the correct build of pnglibconf.h; as it is cmake is
pretty much useless on all but the standard "build everything and hope"
systems.  Aka x86 and amd64...  Deconfiguration is pretty much impossible;
look at the pngminim builds.  Things which are useful on small devices,
like, for example, just building read code, just building the simplified
API, etc work fine with configure and aren't even remotely possible with
cmake simply because of the lack of support for pngusr.dfa.  It should be
an easy fix for a cmake expert.

John Bowler

--0000000000004fd0ae05eac58be7
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div dir=3D"ltr">On Tue, Oct 11, 2022 at 10:05 AM Martin S=
torsj=C3=B6 &lt;<a href=3D"mailto:[email protected]">[email protected]</a>&gt=
; wrote:<br></div><div class=3D"gmail_quote"><div>=C2=A0</div><blockquote c=
lass=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px soli=
d rgb(204,204,204);padding-left:1ex">In the cmake build (contrary to the co=
nfigure script!), <br>
PNG_ARM_NEON_CHECK_SUPPORTED gets defined by default when building for an <=
br>
ARM target, unless the user requested something else.<br></blockquote><div>=
<br></div><div>That is the bug in <i>the cmake build</i>; as the comments p=
oint out cmake is not building pnglibconf.h because neither I nor Glenn cou=
ld work out how to invoke awk from cmake.</div><div><br></div><div>In any c=
ase the &quot;check&quot; option is very, very deprecated and should not be=
 the default anywhere; API is fine.=C2=A0 arm/arm_init.c even complains lou=
dly if it is set on arm64s.</div><div><br></div><blockquote class=3D"gmail_=
quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,=
204);padding-left:1ex">
<br>
The NEON runtime check is implemented for Linux, but not for other OSes, <b=
r>
hitting #error &quot;No support for run-time ARM Neon checking; use <br>
compile-time options&quot; when building for iOS and Windows on armv7.<br><=
/blockquote><div><br></div><div>Yep.=C2=A0 It&#39;s deprecated.</div><div>=
=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0=
.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
While the user could override this when configuring the build, I though it =
<br>
could be avoided - on iOS and Windows on armv7, NEON is part of the <br>
platform baseline, so the compiler is free to use NEON anywhere in code <br=
>
generated from the C code, so no runtime check should be needed. The <br>
compiler signals this, that NEON code generation is enabled <br>
unconditionally, by defining __ARM_NEON__ (or __ARM_NEON).<br></blockquote>=
<div><br></div><div>Nope; you are fixing a cmake bug by breaking an API.=C2=
=A0 Sure, removing the API is fine, but just breaking it because of a simpl=
e failure to update CMakeLists.txt is clearly wrong.</div><div><br></div><d=
iv>Make the obvious changes aroung line 1040 of CMakeLists.txt!.=C2=A0 Or, =
better, use the configure build which is way more up-to-date than the cmake=
 stuff.=C2=A0 For sure the cmake build should not build a different default=
 configuration.=C2=A0 =C2=A0Here&#39;s the default:</div><div><br></div><di=
v>1) PNG_ARM_NEON_OPT <b>should be unset by default.</b>=C2=A0 Otherwise mu=
ltilib builds are broken.</div><div>2) Neither PNG_ARM_NEON_<b>CHECK</b> no=
r <b>API </b>should be defined by default; they should be options.=C2=A0 Bo=
th of them can be set at the same time IRC.</div><div>3) The settings shoul=
d be reflected in the installed pnglibconf.h</div><div>4) Much better; cmak=
e should behave like configure, take the same options with the same default=
s (not invent a new default configuration) and implement these via the corr=
ect build of pnglibconf.h; as it is cmake is pretty much useless on all but=
 the standard &quot;build everything and hope&quot; systems.=C2=A0 Aka x86 =
and amd64...=C2=A0 Deconfiguration is pretty much impossible; look at the p=
ngminim builds.=C2=A0 Things which are useful on small devices, like, for e=
xample, just building read code, just building the simplified API, etc work=
 fine with configure and aren&#39;t even remotely possible with cmake simpl=
y because of the lack of support for pngusr.dfa.=C2=A0 It should be an easy=
 fix for a cmake expert.</div><div><br></div><div>John Bowler</div></div></=
div>

--0000000000004fd0ae05eac58be7--


--===============3382500824848788342==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline


--===============3382500824848788342==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

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

--===============3382500824848788342==--