Allow overriding default flags

[jonasnick]

Right now, it's not easy to reduce the optimization level with CFLAGS because configure overwrites any optimization flag with -O3. The automake documentation states that:

The reason ‘$(CPPFLAGS)’ appears after ‘$(AM_CPPFLAGS)’ or ‘$(mumble_CPPFLAGS)’ in the compile command is that users should always have the last say.

and also that it's incorrect to redefine CFLAGS in the first place

You should never redefine a user variable such as CPPFLAGS in Makefile.am. [...] You should not add options to these user variables within configure either, for the same reason

With this PR CFLAGS is still redefined, but user-provided flags appear after the default CFLAGS which means that they override the default flags (at least in clang and gcc). Otherwise, the default configuration is not changed. This also means that if CFLAGS are defined by the user, then -g is not added (which does not seem to make much sense). In order to keep the -O3 despite the reordering we need to explicitly tell autoconf to not append -O2 by setting the default to -g with : ${CFLAGS="-g"} as per the manual (EDIT: link fix).

[real-or-random]

Approach ACK

In order to keep the -O3 despite the reordering we need to explicitly tell autoconf to not append -O2 by setting the default to -g with : ${CFLAGS="-g"} as per the manual.

I think the relevant section is https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.69/autoconf.html#C-Compiler , macro AC_PROG_CC

[gmaxwell]

This has irritated me a number of times. It's conventional to prepend rather than append.

[real-or-random]

Suggested change

	# Set -g if CFLAGS are not already set
	# Set -g but not -O2 if CFLAGS are not already set (see AC_PROG_CC in the Autoconf manual)

This is obscure enough to deserve a comment.

ACK otherwise

[jonasnick]

fixed

[jonasnick]

Eh, anything starting with AC_ in an error if it occurs in a comment:

configure:3073: error: possibly undefined macro: AC_PROG_CC
      If this token and others are legitimate, please use m4_pattern_allow.
      See the Autoconf documentation.

Now fixed for real

[sipa]

Concept ACK, I don't think the current behavior is intentional at all.

@theuni want to have a look?

[real-or-random]

ACK 83fb1bc

[sipa]

@theuni Would you mind having a look?

[theuni]

Does this end up changing Core's build to -O2, since the -O3 is no longer forced on? I believe our CFLAGS specify -O2, at least for release builds.

Edit: It's fine if that's the case, but presumably we'd want Core to explicitly pass -O3 in order to get the current behavior.

[real-or-random]

Does this end up changing Core's build to -O2, since the -O3 is no longer forced on? I believe our CFLAGS specify -O2, at least for release builds.

Edit: It's fine if that's the case, but presumably we'd want Core to explicitly pass -O3 in order to get the current behavior.

Yes, e.g., in https://github.com/bitcoin/bitcoin/blob/452bb90c718da18a79bfad50ff9b7d1c8f1b4aa3/depends/hosts/mingw32.mk, also in the gitian builds.

We should probably first fix this in Core then. Anyone feel competent to do that change?

[gmaxwell]

It might be better to spend time making code quality improvements to narrow the difference between O3 and O2 and then just be O2 by default.

[theuni]

I could come up with something for Core, but I agree with @gmaxwell as well.

Are the performance differences between -O2 and -O3 generally understood for libsecp256k1? I realize that's a pretty broad question as it spans compilers...

If not, I should think that the "-O3" performance should be achievable with "-O2 -ffoo -fbar" as well, and I could do some quick benches to see which specific flags account for the bulk of the differences. Hopefully that would point to some obvious manual optimizations.

[gmaxwell]

At least some of it is due to autovectorization.

[real-or-random]

I did some quick and dirty benchmarks and I couldn't find a significant difference between -O2 and -O3 on my machine. I tried multiple options (with and without asm, with and without gmp). But this feels too good to be true, maybe something is wrong with my setup. (No, I didn't forget to checkout this PR here :)).

If there is no difference (we care about) then we should simply switch to -O2. Otherwise I'm not sure. In a world with infinite developer time, it's for sure to make manual improvements. But I'm somewhat concerned in the real world this will postpone this PR (which fixes something like a real bug) forever.

[sipa]

This wouldn't surprise me too much. GCC got a lot better since the code was originally written and we decided to pick -O3. It would be useful to not just test x86_64 though.

[jonasnick]

Did a couple of experiments summarized in the following table. There are statistically significant but mostly small differences.

| arch (*) | compiler (**) | gmp | asm | result (***)                                |
|----------+---------------+-----+-----+---------------------------------------------|
| x86_64   | gcc           | yes | yes | sign: +0.1us (0.2%), verify: +0.4us (0.6%)  |
| x86_64   | gcc           | no  | yes | sign: +0.1us (0.2%), verify: -0.5us (-0.7%) |
| x86_64   | gcc           | no  | no  | sign: +0.3us (0.7%), verify: -3.2us (-4.7%) |
| x86_64   | clang         | yes | yes | sign: +0.0us (0.0%), verify: +0.1us (0.1%)  |
| aarch64  | gcc           | yes | no  | sign: +2.0us (0.7%), verify: +5.0us (1.0%)  |
| aarch64  | gcc           | no  | no  | sign: +3.0us (1.1%), verify: -9.0us (-1.7%) |
| aarch64  | clang         | yes | no  | sign: -2.0us (0.6%), verify: +4.0us (0.7%)  |

(*) x86_64: Intel(R) Core(TM) i7-7820HQ CPU max 3.9GHz, aarch64: Cortex A53 max 1.5GHz
(**) gcc 8.3.0, clang 9.0.1
(***) Result is bench_sign and bench_verify --O3 optimization level subtracted from --O2 (so positive means --O3 is faster). benchmark iteration count was increased from 10 to 100.

[real-or-random]

Nice! I think that rather speaks for -O2 then.

Did you also look at the binary sizes, too? I guess -O2 is smaller there.

[jonasnick]

On aarch64 without bignum libsecp256k1.a is 159K (--O3) vs. 154K (--O2).

[theuni]

+1 for just dropping the -O3 stuff, then.

[theuni]

ACK the approach and changes here, btw, excluding the optimization issue.
Thanks for fixing this.

[real-or-random]

on x86_64, gcc (no static precomp):
O2: 88698 bytes
O3: 94722 bytes

Yes, I think we should make -O2 the default. People can still override this if they want -O3.

[real-or-random]

Couldn't we get rid of this entirely then because Autoconf's default is -g -O2?

If your answer is, maybe keep this variant still as a branch somewhere until we have further Concept ACKs on the idea of switching to -O2 :p

[jonasnick]

If we remove this then if $CFLAGS aren't already set they will be set to -O2 ... -O2. The comment additionally serves as a hint to how autoconf works :) because I'd rather set : ${CFLAGS=""} in the future, because anything else doesn't make much sense to me.

[real-or-random]

Okay yes. Also, autoconf sets -O2 apparently only for gcc and not for other compilers according to its manual... So we need to keep the -O2 further below anyway.

[gmaxwell]

I tried multiple options (with and without asm, with and without gmp). But this feels too good to be true,

This is really surprising, previously with ASM disabled O3 got almost as much speedup as enabling asm did. Are you now seeing ASM not making a big performance difference anymore at O2? Is it possible that you're somehow ending up with cached results?

[To be clear, I think moving off O3 is probably a good idea, but it should be done with an understanding of the performance impact in mind... and if it were large, perhaps some bisection to determine what functions had the biggest impact so they could get optimization effort]

[real-or-random]

much speedup as enabling asm did. Are you now seeing ASM not making a big performance dif

@jonasnick's table seems to imply that (on x86_64) O2 is even faster with asm and gmp turned off.

[real-or-random]

@gmaxwell Are you willing to quickly verify our results? I think this is essentially ready to merge if we're confident about the benchmarks.

[theuni]

I pushed a quick hack branch here to help with benching: https://github.com/theuni/secp256k1/commits/run_benches

It switches to clock_gettime to rule out wall-clock/drift issues, bumps the bench iteration to 100 to match @jonasnick's tests, and adds a quick bench-runner script.

Locally I've disabled my cpu's turbo boost and set all governors to "performance". Additionally, the bench runner sets the cpu affinity to match on all tests.

It tests a matrix of gcc/clang versions, configure switches, and optimization flags.

I'll post the results tonight or tomorrow when it's done.

[theuni]

I'll sort/analyze tomorrow if nobody beats me to it. Will repeat on arm as well. One quick spoiler though. The winner on my desktop:

gcc-8 --with-asm=no --with-scalar=64bit --with-field=64bit -O2

AMD Ryzen Threadripper 2970WX. Performance governor puts it at 3ghz.
Cleaned up output:

Using global config: --enable-ecmult-static-precomputation --with-bignum=no --disable-openssl-tests

gcc-7 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -Os
ecdsa_verify: min 118604ns / avg 118639ns / max 118686ns

gcc-7 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -O2
ecdsa_verify: min 108535ns / avg 108591ns / max 108662ns

gcc-7 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -O3
ecdsa_verify: min 108905ns / avg 108936ns / max 108979ns

gcc-7 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -Ofast
ecdsa_verify: min 108969ns / avg 109005ns / max 110088ns

gcc-7 --with-asm=no --with-scalar=64bit --with-field=64bit -Os
ecdsa_verify: min 126077ns / avg 126628ns / max 126854ns

gcc-7 --with-asm=no --with-scalar=64bit --with-field=64bit -O2
ecdsa_verify: min 113199ns / avg 113212ns / max 113235ns

gcc-7 --with-asm=no --with-scalar=64bit --with-field=64bit -O3
ecdsa_verify: min 113855ns / avg 113996ns / max 120455ns

gcc-7 --with-asm=no --with-scalar=64bit --with-field=64bit -Ofast
ecdsa_verify: min 113168ns / avg 113185ns / max 113207ns

gcc-8 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -Os
ecdsa_verify: min 118461ns / avg 118571ns / max 118642ns

gcc-8 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -O2
ecdsa_verify: min 106635ns / avg 106666ns / max 106704ns

gcc-8 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -O3
ecdsa_verify: min 107805ns / avg 107830ns / max 108036ns

gcc-8 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -Ofast
ecdsa_verify: min 107810ns / avg 108053ns / max 112453ns

gcc-8 --with-asm=no --with-scalar=64bit --with-field=64bit -Os
ecdsa_verify: min 125447ns / avg 125699ns / max 132602ns

gcc-8 --with-asm=no --with-scalar=64bit --with-field=64bit -O2
ecdsa_verify: min 105002ns / avg 105066ns / max 110299ns

gcc-8 --with-asm=no --with-scalar=64bit --with-field=64bit -O3
ecdsa_verify: min 112369ns / avg 112528ns / max 119829ns

gcc-8 --with-asm=no --with-scalar=64bit --with-field=64bit -Ofast
ecdsa_verify: min 112420ns / avg 112430ns / max 112449ns

clang-6.0 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -Os
ecdsa_verify: min 108773ns / avg 108919ns / max 109015ns

clang-6.0 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -O2
ecdsa_verify: min 108381ns / avg 108404ns / max 108429ns

clang-6.0 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -O3
ecdsa_verify: min 108207ns / avg 108225ns / max 108284ns

clang-6.0 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -Ofast
ecdsa_verify: min 108276ns / avg 108293ns / max 108386ns

clang-6.0 --with-asm=no --with-scalar=64bit --with-field=64bit -Os
ecdsa_verify: min 119659ns / avg 119890ns / max 123911ns

clang-6.0 --with-asm=no --with-scalar=64bit --with-field=64bit -O2
ecdsa_verify: min 117406ns / avg 117645ns / max 127949ns

clang-6.0 --with-asm=no --with-scalar=64bit --with-field=64bit -O3
ecdsa_verify: min 115289ns / avg 115734ns / max 116006ns

clang-6.0 --with-asm=no --with-scalar=64bit --with-field=64bit -Ofast
ecdsa_verify: min 115479ns / avg 115696ns / max 115969ns

clang-7 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -Os
ecdsa_verify: min 109078ns / avg 109222ns / max 109610ns

clang-7 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -O2
ecdsa_verify: min 108055ns / avg 108095ns / max 108376ns

clang-7 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -O3
ecdsa_verify: min 108176ns / avg 108217ns / max 108264ns

clang-7 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -Ofast
ecdsa_verify: min 108207ns / avg 108222ns / max 108238ns

clang-7 --with-asm=no --with-scalar=64bit --with-field=64bit -Os
ecdsa_verify: min 112558ns / avg 112778ns / max 123525ns

clang-7 --with-asm=no --with-scalar=64bit --with-field=64bit -O2
ecdsa_verify: min 109983ns / avg 110230ns / max 119333ns

clang-7 --with-asm=no --with-scalar=64bit --with-field=64bit -O3
ecdsa_verify: min 108829ns / avg 108928ns / max 108972ns

clang-7 --with-asm=no --with-scalar=64bit --with-field=64bit -Ofast
ecdsa_verify: min 110449ns / avg 111127ns / max 124666ns

clang-8 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -Os
ecdsa_verify: min 108850ns / avg 109174ns / max 111568ns

clang-8 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -O2
ecdsa_verify: min 108154ns / avg 108197ns / max 108234ns

clang-8 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -O3
ecdsa_verify: min 108208ns / avg 108236ns / max 108266ns

clang-8 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -Ofast
ecdsa_verify: min 108177ns / avg 108198ns / max 108280ns

clang-8 --with-asm=no --with-scalar=64bit --with-field=64bit -Os
ecdsa_verify: min 112673ns / avg 112833ns / max 119882ns

clang-8 --with-asm=no --with-scalar=64bit --with-field=64bit -O2
ecdsa_verify: min 111448ns / avg 114072ns / max 128824ns

clang-8 --with-asm=no --with-scalar=64bit --with-field=64bit -O3
ecdsa_verify: min 108672ns / avg 108699ns / max 108768ns

clang-8 --with-asm=no --with-scalar=64bit --with-field=64bit -Ofast
ecdsa_verify: min 108354ns / avg 108391ns / max 108509ns

clang-9 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -Os
ecdsa_verify: min 108979ns / avg 109332ns / max 111432ns

clang-9 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -O2
ecdsa_verify: min 108060ns / avg 108094ns / max 108400ns

clang-9 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -O3
ecdsa_verify: min 108029ns / avg 108044ns / max 108074ns

clang-9 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -Ofast
ecdsa_verify: min 108175ns / avg 108212ns / max 108249ns

clang-9 --with-asm=no --with-scalar=64bit --with-field=64bit -Os
ecdsa_verify: min 110937ns / avg 111384ns / max 111919ns

clang-9 --with-asm=no --with-scalar=64bit --with-field=64bit -O2
ecdsa_verify: min 108739ns / avg 108756ns / max 108830ns

clang-9 --with-asm=no --with-scalar=64bit --with-field=64bit -O3
ecdsa_verify: min 107166ns / avg 107202ns / max 107242ns

clang-9 --with-asm=no --with-scalar=64bit --with-field=64bit -Ofast
ecdsa_verify: min 107047ns / avg 107064ns / max 107116ns

[gmaxwell]

Can you take two interesting configs, like -O3 with asm, and O2 no asm, and step back through a number of historical versions to see if changes to the library (or the benchmark) changed the results? (I'm kinda wondering if the benchmarks didn't get messed up.)

Old chatlogs suggested that there were enormous differences that justified O3 in the first place. The PR that added scalar asm claims it was at the time a 12% speedup for signing, etc.

[real-or-random]

@gmaxwell I tried to do this and I couldn't find a revision where O3asm is really better for verification than O2. Maybe the difference is more significant for signing. I checked signing only later.

Here's a way too long but helpful command that's pretty robust across revisions. But you should make sure to grep the Makefile for CFLAGS to make sure it's really doing the right thing.
sed -i 's/-O3//g' configure.ac && ./autogen.sh && ./configure --enable-benchmark --with-bignum=no --with-asm=no --disable-ecmult-static-precomputation --disable-tests --disable-exhaustive-tests CFLAGS="-O2" && make clean && make && mv -f bench_verify bench_verify_O2 && mv -f bench_sign bench_sign_O2 && ./configure --enable-benchmark --with-bignum=no --with-asm=x86_64 --disable-ecmult-static-precomputation --disable-tests --disable-exhaustive-tests CFLAGS="-O3" && make clean && make && mv -f bench_verify bench_verify_O3asm && mv -f bench_sign bench_sign_O3asm && ./bench_verify_O2 && ./bench_verify_O3asm && ./bench_sign_O2 && ./bench_sign_O3asm ; git checkout -- configure.ac

Your example of #161 (asm for scalar):
Before #161:

O2    min 58.544us / avg 59.346us / max 61.378us
O3    min 57.509us / avg 58.296us / max 59.643us

After #161:

O2    min 58.603us / avg 59.665us / max 61.372us
O3asm min 55.958us / avg 56.943us / max 58.980us

So the speedup introduced by this PR is only ~2% on my machine with gcc 9. Back then people used gcc 4, so maybe compilers really got that much better for our code.

[theuni]

For posterity, I used gcc's "-Q --help=optimizers" options to find the actual difference between O2 and O3, so we can speak in terms of functionality rather than black boxes :)

For gcc 8 on x86_64, this should be equivalent to using -O3:

./configure CFLAGS="-O2 -fgcse-after-reload -finline-functions -fipa-cp-clone -floop-interchange -floop-unroll-and-jam -fpeel-loops -fpredictive-commoning -fsplit-loops -fsplit-paths -ftree-loop-distribute-patterns -ftree-loop-distribution -ftree-loop-vectorize -ftree-partial-pre -ftree-slp-vectorize -funswitch-loops -fvect-cost-model=dynamic"

I think we've beaten the topic to death here, but someone down the road might be interested in looking at turning on/off optimizations with more precision. It's interesting to think that could be a better use of time than just writing optimized asm.

[sipa]

The main thing that the hand-rolled assembly helps with is register allocation. When it was written, the compiler-emitted code made some silly decisions to spill intermittently (but often) used values to the stack and load them every time.

It's possible that GCC (and other compilers) have just improved enough to the point that the benefits of handrolled asm is much smaller.

[real-or-random]

Even if we don't know the reasons exactly, the benchmarks show that switching to O2 is a reasonable thing to do, so we should do this.

ACK ca739cb

[theuni]

ACK ca739cb.

I'll follow-up with a PR to move away from modifying CFLAGS itself. @jonasnick is exactly right, that's nasty.

[elichai]

Just realized that until this is merged the last test in the matrix with EXTRAFLAGS=CFLAGS=-O0 is useless, because it still adds -O3 at the end. https://travis-ci.org/elichai/secp256k1/jobs/658344952#L388

[elichai]

ACK ca739cb

[hebasto]

83fb1bc

This causes multiple rm: conftest.dSYM: is a directory warnings on macOS (#896).

[hebasto]

This also means that if CFLAGS are defined by the user, then -g is not added (which does not seem to make much sense).

Do I understand correctly that the intention is to pass -g flag to a compiler regardless of flags provided by a user?

[jonasnick]

IIRC no, this PR makes it so that -g is passed if the user doesn't provide custom CFLAGS.

[real-or-random]

So 83fb1bc means that the flags used during the tests are simply "-g" (instead of "-g -O2"), and this is what makes Apple's fake gcc creating the directory. I believe we should simply set : ${CFLAGS=""} here. Or if we want to keep the -g as a default, we could remember whether CFLAGS was unset, and if not, add -g later.

Do we want to keep the -g default?