[CI test, dontmerge] PR #1000 with __(u)mul128() based abstractions in int128_struct

[sipa]

No description provided.

[real-or-random]

I know this is just a CI test but I think we want something like this after #1000. Then I'd suggest this as a heuristic:

#elif UINTPTR_MAX >= UINT64_MAX

[sipa]

That doesn't guarantee the existence of __mulh and __umulh?

[real-or-random]

No, it doesn't. But even without access to an efficient 64x64->128 mul, it's still a good idea to use the int128_struct implementation on a 64bit machine (with the simulated 64x64->128 mul then) because then the 64-bit field/scalar code will be used.

(I admit I don't know if we'll run into such a machine/compiler. It may be a good idea to output a preprocessor warning in this case, or actually remove the simulated 64x64->128 mul code because our tests will never exercise it.)

[sipa]

Hmm, perhaps we should benchmark the 10x26 field code against the 5x52 + int128_struct code. I'm not convinced about which one is faster.

[real-or-random]

It's faster on 64 bit platforms because all the other operations except muls will profit:

Intel(R) Core(TM) i7-7600U CPU @ 2.80GHz (x86_64), gcc 12.1.0 -O2

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

./configure --with-test-override-wide-multiply=int64

ecdsa_verify                  ,    95.3       ,    96.2       ,   102.0    
ecdsa_sign                    ,    64.0       ,    64.1       ,    64.7  

./configure  --with-test-override-wide-multiply=int128_struct

ecdsa_verify                  ,    55.9       ,    56.4       ,    57.4    
ecdsa_sign                    ,    42.2       ,    42.4       ,    43.0    

./configure 

ecdsa_verify                  ,    54.9       ,    55.4       ,    56.1    
ecdsa_sign                    ,    39.9       ,    40.1       ,    40.7    

[roconnor-blockstream]

We should probably change this right?

[sipa]

Change it to what? There are instrinsics for some platforms to do addition-with-carry, which likely result in better performance here (as well as constant-time operation), but they're obviously not standard C.

[roconnor-blockstream]

I understood there were similar non-constant time issues in libsecp256k1 where comparisons were being transformed into branches, and that refactoring the code somehow fixed the problem.

[sipa]

Maybe, but I'm not sure that's needed.

All of this is empirical of course - it's very much possible for vaguely reasonable compilers to convert code like this into either constant time or non-constant time code.

But based on what I've seen from GCC, this isn't too suprising: i686 doesn't have a (CPU) native 64-bit type; all (u)int64_t operations are effectively already translated to operations on pairs of 32-bit integers. And it seems that on several platforms, GCC compiles comparisons on tuple-represented integers to branches (this is also true on 64-bit platforms with __int128).

Code of this form already occurs in scalar_4x64_impl.h, but it's not a problem there, because it's only used on 64-bit platforms, where uint64_t isn't represented as a pair. For the same reason, it does not seem to be a problem if int128_struct_impl.h is used on 64-bit systems.

[real-or-random]

all (u)int64_t operations are effectively already translated to operations on pairs of 32-bit integers. And it seems that on several platforms, GCC compiles comparisons on tuple-represented integers to branches

That makes a lot of sense but recent GCC >=8 seems to get away without a branch, at least in this simple test... https://gcc.godbolt.org/z/nss58G3Ko

If we find a nice of rewriting these comparisons, fine. But I don't see any way of fixing this, so I wouldn't bother too much. The build is seriously misconfigured if you use int128_struct on a 32bit machine, and we don't need to support this.

[roconnor-blockstream]

It may be a good idea to output a preprocessor warning in this case, or actually remove the simulated 64x64->128 mul code because our tests will never exercise it.)

We could add a #warning pragma saying "Beware of bugs in the above code; We have only proved it correct, not tested it."

[sipa]

Superseded by #1156.