Re: [PATCH] cmake: Default to PNG_ARM_NEON=off for arm targets
John Bowler <[email protected]> Fri, 14 Oct 2022 13:16:30 -0700
| Newsgroups | gmane.comp.graphics.png.devel |
|---|---|
| Message-ID | <CAP7U39-4kuwhONiMg=vwGr=ath68DMmd5=1AXZX-N5Cou8FQNQ@mail.gmail.com> |
--===============0412957342551212160== Content-Type: multipart/alternative; boundary="000000000000d484a305eb044d8e" --000000000000d484a305eb044d8e Content-Type: text/plain; charset="UTF-8" I checked on arm64 Ubuntu and my Intel system. Using the current libpng16 HEAD with the patch for Intel both cmake and configure behave as I expect (with no arguments). The comment in the git change log is wrong, "cmake: Default to PNG_ARM_NEON=off for arm targets". The "CACHE STRING" values in CMakeLsits.txt are also wrong and the strings for the other hardware options are either wrong or extremely misleading. They should be the same as the comment in configure --help, which is repeated for each hardware option (copy'n'paste I assume): "If not specified: determined by the compiler." John Bowler <[email protected]> +1 (541) 450-9885 PO BOX 3151 KERBY OR 97531-3151 USA On Fri, Oct 14, 2022 at 11:07 AM John Bowler < [email protected]> wrote: > On Fri, Oct 14, 2022 at 4:34 AM Cosmin Truta <[email protected]> wrote: > >> On Thu, Oct 13, 2022 at 9:48 PM John Bowler >> <[email protected]> wrote: >> > I double checked. It is exactly as I said; if you do not pass *any* >> command line arguments to configure you get the relevant (target) CPU >> optimizations compiled in so long as the compiler supports them. (The >> latter check happens in pngpriv.h, which, as I said, is probably the wrong >> place.) >> >> The same goes for the CMake file. I actually tested it. >> > > Good. It might be useful to flag any attempt to use these options apart, > maybe, from the "turn all optimisations off". Forcing the hardware stuff > on is very dangerous and likely to lead to compile time failure with > different compilers. It's only useful for developers working on hardware > stuff who don't, yet, have adequate compiler support. In the past people > have misunderstood and assumed it was necessary to pass an --enable option > to configure to turn it on, failing to realise that it happens > automagically by default. > > IRC the *--enable-hardware-optimizations* configure option that is right > at the start of this block of configure stuff was intended to replace all > the other options in a future release. > > >So, John, to conclude your point: do you agree that Martin's patch does > the right thing, or do you not? And if not, then what is the correct > solution in your opinion? > > I don't know enough about cmake to comment on that; I was responding to > Martin's observation at the start of this thread. He said: > > >This matches the configure script - unless the user has requested > anything, no hardware optimizations are autoenabled. > > And *that *is certainly *not* the behavior of configure. If the user > just types: > > > .../configure > > (I.e. no arguments, "not requesting anything") then the compiler checks > are run in pngpriv.h and the optimisations are turned on if the compiler > supports them because the relevant _OPT symbol is not defined. This > applies to NEON, VSX and MIPS; INTEL is broken because of the spurious > check in a C file on an AM_CONDITIONAL... I unborked it and it works fine. > > So there is disagreement between what you say cmake does and what Martin > says it does. Your description is the correct behavior so assuming Martin > is wrong everything seems fine. It's easy to check; just do > configure/cmake then make; if it's working correctly then the "arm" > subdirectory will contain .o files and "nm" will show they have the right > symbols. > > >It is worth introducing a new API function that informs the user about > the available optimizations. > > No. What is an app going to do with that information other than something > that causes problems? It's just feature creep/code bloat. > > For configure: > > .../configure --disable-hardware-optimizations # no hardware specific > files should be built, or if they are built they should be empty > .../configure --enable-hardware-optimizations # the platform specific > hardware should be built, but it may fail with non-gcc compilers (try > clang...) > .../configure # should *always* work, and should build the hardware opts > if the previous step was able to > > > > > --000000000000d484a305eb044d8e Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable <div dir=3D"ltr">I checked on arm64 Ubuntu and my Intel system.=C2=A0 Using= the current libpng16 HEAD with the patch for Intel both cmake and configur= e behave as I expect (with no arguments).<div><br></div><div>The comment in= the git change log is wrong, "cmake: Default to PNG_ARM_NEON=3Doff fo= r arm targets".=C2=A0 The "CACHE STRING" values in CMakeLsit= s.txt are also wrong and the strings for the other hardware options are eit= her wrong or extremely misleading.=C2=A0 They should be the same as the com= ment in configure --help, which is repeated for each hardware option (copy&= #39;n'paste I assume): "If not specified: determined by the compil= er."<br><div><br clear=3D"all"><div><div dir=3D"ltr" class=3D"gmail_si= gnature" data-smartmail=3D"gmail_signature"><div dir=3D"ltr"><div><div>John= Bowler <<a href=3D"mailto:[email protected]" target=3D"_blank">jbowler@ac= m.org</a><font color=3D"#000120"></font><u></u>></div><div>+1 (541) 450-= 9885</div><div>PO BOX 3151</div><div>KERBY OR 97531-3151</div><div>USA</div= ></div></div></div></div><br></div></div></div><br><div class=3D"gmail_quot= e"><div dir=3D"ltr" class=3D"gmail_attr">On Fri, Oct 14, 2022 at 11:07 AM J= ohn Bowler <<a href=3D"mailto:[email protected]">john.cun= [email protected]</a>> wrote:<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"><div dir=3D"ltr"><div dir=3D"ltr">On Fri, Oct 14, 2= 022 at 4:34 AM Cosmin Truta <<a href=3D"mailto:[email protected]" target= =3D"_blank">[email protected]</a>> wrote:<br></div><div class=3D"gmail_qu= ote"><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;bo= rder-left:1px solid rgb(204,204,204);padding-left:1ex">On Thu, Oct 13, 2022= at 9:48 PM John Bowler<br> <<a href=3D"mailto:[email protected]" target=3D"_blank">j= [email protected]</a>> wrote:<br> > I double checked.=C2=A0 It is exactly as I said; if you do not pass *a= ny* command line arguments to configure you get the relevant (target) CPU o= ptimizations compiled in so long as the compiler supports them.=C2=A0 (The = latter check happens in pngpriv.h, which, as I said, is probably the wrong = place.)<br> <br> The same goes for the CMake file. I actually tested it.<br></blockquote><di= v><br></div><div>Good.=C2=A0 It might be useful to flag any attempt to use = these options apart, maybe, from the "turn all optimisations off"= .=C2=A0 Forcing the hardware stuff on is very dangerous and likely to lead = to compile time failure with different compilers.=C2=A0 It's only usefu= l for developers working on hardware stuff who don't, yet, have adequat= e compiler support.=C2=A0 In the past people have misunderstood and assumed= it was necessary to pass an --enable option to configure to turn it on, fa= iling to realise that it happens automagically by default.</div><div><br></= div><div>IRC the <b>--enable-hardware-optimizations</b> configure option th= at is right at the start of this block of configure stuff was intended to r= eplace all the other options in a future release.</div><div><br></div><div>= >So, John, to conclude your point: do you agree that Martin's patch = does the right thing, or do you not? And if not, then what is the correct s= olution in your opinion?<br></div><div><br></div><div>I don't know enou= gh about cmake to comment on that; I was responding to Martin's observa= tion at the start of this thread.=C2=A0 He said:</div><div><br></div><div>&= gt;This matches the configure script - unless the user has requested anythi= ng, no hardware optimizations are autoenabled.</div><div><br></div><div>And= <b>that </b>is certainly <b>not</b> the behavior of configure.=C2=A0 If th= e user just types:</div><div><br></div><div>> .../configure</div><div><b= r></div><div>(I.e. no arguments, "not requesting anything") then = the compiler checks are run in pngpriv.h and the optimisations are turned o= n if the compiler supports them because the relevant _OPT symbol is not def= ined.=C2=A0 This applies to NEON, VSX and MIPS; INTEL is broken because of = the spurious check in a C file on an AM_CONDITIONAL...=C2=A0 I unborked it = and it works fine.<br></div><div><br></div><div>So there is disagreement be= tween what you say cmake does and what Martin says it does.=C2=A0 Your desc= ription is the correct behavior so assuming Martin is wrong everything seem= s fine.=C2=A0 It's easy to check; just do configure/cmake then make; if= it's working correctly then the "arm" subdirectory will cont= ain .o files and "nm" will show they have the right symbols.<br><= /div><div><br></div><div>>It is worth introducing a new API function tha= t informs the user about the available optimizations.</div><div><br></div><= div>No.=C2=A0 What is an app going to do with that information other than s= omething that causes problems?=C2=A0 It's just feature creep/code bloat= .</div><div><br></div><div>For configure:</div><div><br></div><div>.../conf= igure --disable-hardware-optimizations # no hardware specific files should = be built, or if they are built they should be empty</div><div>.../configure= --enable-hardware-optimizations=C2=A0 # the platform specific hardware sho= uld be built, but it may fail with non-gcc compilers (try clang...)</div><d= iv>.../configure # should *always* work, and should build the hardware opts= if the previous step was able to</div><div><br></div><div><br></div><div><= br></div><div><br></div></div></div> </blockquote></div> --000000000000d484a305eb044d8e-- --===============0412957342551212160== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline --===============0412957342551212160== 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 --===============0412957342551212160==--