Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 4 commits into from
Closed

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

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8cf4667
Simulated int128 type.
roconnor-blockstream Jul 11, 2022
62554ab
int128: Tidy #includes of int128.h and int128_impl.h
real-or-random Feb 1, 2022
2fe03cd
Switch to int128_struct implementation using (u)mul128 abstractions
sipa Jul 22, 2022
412f359
Use int128_struct on 64-bit MSVC
sipa Jul 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,9 @@ static SECP256K1_INLINE void secp256k1_int_cmov(int *r, const int *a, int flag)
#elif defined(UINT128_MAX) || defined(__SIZEOF_INT128__)
# define SECP256K1_WIDEMUL_INT128 1
# define SECP256K1_INT128_NATIVE 1
#elif defined(_MSC_VER) && (defined(_M_X64) || defined(_M_ARM64))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@real-or-random real-or-random Jul 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably change this right?

Copy link
Contributor Author

@sipa sipa Aug 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@sipa sipa Aug 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# define SECP256K1_WIDEMUL_INT128 1
# define SECP256K1_INT128_STRUCT 1
#else
# define SECP256K1_WIDEMUL_INT64 1
#endif
Expand Down