Synthetic int128 type.

[roconnor-blockstream]

Abstracts the int128 type and provides an native version, if available, or a implements it using a pair of int64_t's.

This is activated by setting the configuration flag --with-test-override-wide-multiply=int128_struct.

The primary purpose of this PR is to take advantage of MSVC's umulh intrinsic that we can use to simulate an int128 type which MSVC does not have (AFAIU). This PR lays out the groundwork for this level of MSVC support, but doesn't include the configuration logic to enable it yet.

For completeness, and implementation of umulh and mulh are also provided for compilers that support neither the intrinsic nor the int128 type (such as CompCert?). This also opens up the possibility of removing the 32-bit field and scalar implementations should that ever be desired.

[real-or-random]

I think it would help readability to arrange the different implementations of the type in different files, similar to what we have a field and scalar "modules", where we have implementation files such as scalar4x64_impl.h but still a single scalar.h that makes sure that all function prototypes are identical. Then you could remove a lot of the ifdefs within the functions.

[roconnor-blockstream]

I'm moving this out of draft stage. The coding is complete. There are a few tasks that remain.

• Double check that there is no signed integer overflow due to rearrangement of the order of some operations.
• We need someone to build this on MSVC.
• Make sure that the refactored code for the native int128 type isn't any slower.

To compile with the synthetic int128 type, you currently need to pass a configuration flag --with-test-override-wide-multiply=int128_struct.

[real-or-random]

* [ ]  We need someone to build this on MSVC.

I have built this with MSVC 2019 on wine (https://github.com/mstorsjo/msvc-wine) and it works. Tests and exhaustive tests pass.

For reference:

 ./configure --disable-benchmark --with-test-override-wide-multiply=int128_struct CC=/opt/msvc/bin/x64/cl.exe CFLAGS="-Za -O2 -w" LD=/opt/msvc/bin/x64/link.exe

For the Arch Linux users, there's an AUR package: https://aur.archlinux.org/packages/msvc-wine-git/

[sipa]

FWIW, bitcoin core has an AppVeyor MSVC/Windows CI environment. Maybe it's worth looking into adding one for libsecp256k1 directly.

[real-or-random]

Indeed. Let me note that also Cirrus apparently offers Windows though I have no idea if it's good and/or reliable: https://cirrus-ci.org/guide/windows/

[real-or-random]

Suggested change

	#if defined(_M_X64) | defined(_M_ARM64) | defined(_WIN64) /* MSVC */
	#include <intrin.h>
	#define secp256k1_umulh __umulh
	#define secp256k1_mulh __mulh
	#else
	#if defined(_M_X64) | defined(_M_ARM64) | defined(_WIN64) /* MSVC */
	#error
	#include <intrin.h>
	#define secp256k1_umulh __umulh
	#define secp256k1_mulh __mulh
	#else

Follow-up: I confirmed that change results in an error in the the MSVC build, so the intrinsics should indeed be used.

[real-or-random]

My above comment about the inclusion stuff was wrong but here's a cleaner version:
https://github.com/real-or-random/secp256k1/commits/202201-int128-includes

The first commit simply fixes naming of header guards and should belong to this PR.

The second commit changes to code adheres to what I wrote in #1039:

After this commit, int128.h and int128_impl.h are included as follows:

• .c files which use int128 include int128_impl.h (after util.h)
• .h files which use int128 include int128.h (after util.h)

This list is exhaustive. util.h needs to included first because it sets
up necessary #defines.

If you want, please pick the second commit, too. Or if you don't want to deal with the C mess, I can create a PR with the second commit on top of yours, and we fix this once your PR has been merged.

[roconnor-blockstream]

I notice that your PR still keeps the USE_*_WIDEMUL_* in util.h. it still seems we cannot really move it out of util.h because it both selects between INT128_STRUCT and INT128_NATIVE, but can also select WIDEMUL_INT64 which isn't really INT128 related.

More specifically this block of CPP code is interpreting the wideMultiply configuration option, which I've designed to have 3 options (by adding that third option).

[real-or-random]

I notice that your PR still keeps the USE_*_WIDEMUL_* in util.h. it still seems we cannot really move it out of util.h because it both selects between INT128_STRUCT and INT128_NATIVE, but can also select WIDEMUL_INT64 which isn't really INT128 related.

Right. I think that's fine.

So this means we have a couple of input #defines (preset in the compiler or set by autoconf, e..g, USE_*_WIDEMUL_*) and we have some CPP logic in file that turns those inputs into "nice" #ifdefs that can then be used in the rest of the code base, e.g., INT128_NATIVE. I believe that's a reasonable way of doing things.

(In the future we should move the CPP logic from util.h to a separate file. It somehow ended up in util.h though it does not belong there...)

[roconnor-blockstream]

I've reviewed the changes in the association of + for signed int128 calculations to look for changes in overflow behavior.

Signed int128 is only used in the src/modinv64_impl.h file. The only changes in association occur in the functions:

• secp256k1_modinv64_update_de_62
• secp256k1_modinv64_update_fg_62
• secp256k1_modinv64_update_fg_62_var

The values being summed are all of the form (int128_t)u * ??? + (int128_t)v * ??? (or similarly for q and r). The only way the new association could cause an overflow (or underflow) is if this previous calculation depended on some cancellation within the above addition term to prevent the final accumulation from itself overflowing. However, the previous calculation does not depend on any cancellation. The only assumed constraint on u and v (resp. q and r) is that |u|+|v| <= 2^62, which implies nothing about the values of u and v causing cancellation.

My conclusion, after the above and re-reviewing the above functions, is that neither the new (or old) order of addition risks overflow.

[real-or-random]

Indeed, we know nothing about the signs of the summands. To convince myself, I also redid the bounds analysis:

Counting value bits (i.e., bits that are not the sign bit):

For each step, cd starts with at most 52 bits.
Then we accumulate (int128_t)u * ???, where the factors have at most 62 bits (implied by |u|+|v| <= 2^62*) and 63 bits (trivial bound for int64_t).
Same with (int128_t)v * e0.
Then we accumulate (int128_t) modinfo->modulus.??? * md, where the factors have at most 62 (modulus is in signed62) and 63 value bits.

We work with signed, so the value with the maximum absolute value representable in B bits is -(1<<B) (with absolute 1<<B).
This means the maximum absolute value cd can take is (1<<52) + (1<<62) * (1<<63) + (1<<62) * (1<<63) + (1<<62) * (1<<63) which is a 127 bit value and thus reprensentable in 127 value bits.

Same is true for ce.

*except u==2^62, which needs 63 bits but then v==0, which also works: (1<<52) + (1<<63) * (1<<63) + 0 * (1<<63) + (1<<62) * (1<<63) is also a 127 bit value.

[real-or-random]

@roconnor-blockstream Can you rebase this? This will ease benchmarking against master.

[real-or-random]

Native 128bit performance looks good:

$ SECP256K1_BENCH_ITERS=1000000 ./bench_internal inverse

gcc 11.2, pr:

Benchmark                     ,    Min(us)    ,    Avg(us)    ,    Max(us)

scalar_inverse                ,     2.40      ,     2.41      ,     2.42   
scalar_inverse_var            ,     1.74      ,     1.75      ,     1.81   
field_inverse                 ,     2.38      ,     2.39      ,     2.40   
field_inverse_var             ,     1.73      ,     1.73      ,     1.74

gcc 11.2, master:

Benchmark                     ,    Min(us)    ,    Avg(us)    ,    Max(us)    

scalar_inverse                ,     2.39      ,     2.39      ,     2.41   
scalar_inverse_var            ,     1.77      ,     1.77      ,     1.78   
field_inverse                 ,     2.37      ,     2.38      ,     2.39   
field_inverse_var             ,     1.76      ,     1.77      ,     1.82


clang 13.0.1, pr:
Benchmark                     ,    Min(us)    ,    Avg(us)    ,    Max(us)    

scalar_inverse                ,     2.70      ,     2.70      ,     2.72   
scalar_inverse_var            ,     1.69      ,     1.70      ,     1.70   
field_inverse                 ,     2.69      ,     2.70      ,     2.70   
field_inverse_var             ,     1.69      ,     1.69      ,     1.70   

clang 13.0.1, master:

Benchmark                     ,    Min(us)    ,    Avg(us)    ,    Max(us)    

scalar_inverse                ,     2.69      ,     2.70      ,     2.71   
scalar_inverse_var            ,     1.69      ,     1.69      ,     1.70   
field_inverse                 ,     2.69      ,     2.69      ,     2.69   
field_inverse_var             ,     1.68      ,     1.68      ,     1.68

But this is with asm on, I should have turned it off... Would be nice to see more benchmarks.

[real-or-random]

I haven't reviewed the struct implementations of secp256k1_?128_accum/mul/mulh/rshift algorithms in detail. I think it will be good to have some randomized unit tests here. (Maybe it's ok to just run them when we have a native type too and compare the result?). Most of these functions should be exercised by the current tests already but some may be not. For example, the shift functions are only called with specific shifts, so I think some branches will never be taken.

[real-or-random]

I think we should detect 64-bit MSVC here and select SECP256K1_INT128_STRUCT.

[roconnor-blockstream]

I'd prefer that done in a separate PR.

[real-or-random]

Ok sure, that will make things easier. 👍

edit: If the struct implementation is anyway only enabled with the right configure flag, we can also postpone the #if defined(_M_X64) | defined(_M_ARM64) | defined(_WIN64) discussion and figure the best macros out later.

[real-or-random]

I think we should detect 64-bit MSVC here and select SECP256K1_INT128_STRUCT.

Note for the future PR: What we should actually do is to use to word size to decide which implementation to use, independently of the exact compiler.

[sipa]

I wrote some randomized tests for the int128 functions: https://github.com/sipa/secp256k1/commits/202211_int128

Not everything is covered, but the most tricky functions are.

[real-or-random]

@roconnor-blockstream We could add a configure/config flag for forcing the use of _mulh/_umulh over _mul128/_umul128, and set it for one of our MSVC CI configs.

https://github.com/real-or-random/secp256k1/tree/202211-int128-mulh-override implemented here, ready to be cherry-picked

edit: I've tested this on local MSVC on wine with

./configure --enable-dev-mode --host=x86_64-w64-mingw32 --with-test-override-wide-multiply=int128_struct CPPFLAGS="-DSECP256K1_MSVC_MULH_TEST_OVERRIDE" CC=/opt/msvc/bin/x64/cl CFLAGS="-nologo -diagnosti
cs:caret" LDFLAGS="-XCClinker -nologo -XCClinker -diagnostics:caret" NM="/opt/msvc/bin/x64/dumpbin -symbols -headers" AR="/opt/msvc/bin/x64/lib"

[sipa]

I wrote some randomized tests for the int128 functions: https://github.com/sipa/secp256k1/commits/202211_int128

Not everything is covered, but the most tricky functions are.

Update: now all functions are covered.

[sipa]

ACK a340d95

I think we can deal with the proposed follow-ups in future PRs.

[jonasnick]

ACK a340d95

[sipa]

Just for reference, I redid the aarch64 benchmarks from #1000 (comment) on more modern hardware (Apple M1, and Amazon's Graviton 3). In both cases, int128 was fastest, followed by int64, and int128_struct last.

The actual numbers from Amazon g7 instances (which are based on Graviton 3).

Benchmark                     ,    Min(us)    ,    Avg(us)    ,    Max(us)    

int128
ecdsa_verify                  ,    48.6       ,    48.7       ,    48.9    
ecdsa_sign                    ,    33.6       ,    33.6       ,    33.6    

int64
ecdsa_verify                  ,    60.8       ,    60.8       ,    60.9    
ecdsa_sign                    ,    44.0       ,    44.0       ,    44.0    

int128_struct
ecdsa_verify                  ,   105.0       ,   105.0       ,   105.0    
ecdsa_sign                    ,    58.2       ,    58.2       ,    58.2

Numbers on Apple M1:

Benchmark                     ,    Min(us)    ,    Avg(us)    ,    Max(us)    

int128
ecdsa_verify                  ,    31.4       ,    31.5       ,    32.0    
ecdsa_sign                    ,    23.2       ,    23.3       ,    23.5    

int64
ecdsa_verify                  ,    54.6       ,    54.8       ,    55.5    
ecdsa_sign                    ,    34.0       ,    34.0       ,    34.2    

int128_struct
ecdsa_verify                  ,    83.4       ,    83.9       ,    87.3    
ecdsa_sign                    ,    45.4       ,    45.5       ,    45.5 

[roconnor-blockstream]

Although this file should never be included when SECP256K1_WIDEMUL_INT128 is not defined, we should have an error message here just in case that situation arises. Otherwise confusing error messages happen.

[real-or-random]

This file is actually included unconditionally in secp256k1.c. (One could argue that this is a bad idea, but the nice thing is that it keeps the preprocessor logic in the int128 "module".)