I wanted to experiment with what would be required to split up secp256k1_i128_to_i64 between those cases when a signed 64 bit value is being demoted, versus an unsigned 64 bit value is being extracted from the lower bits, and this is the result.
I’m not sure this is a useful PR, so feel free to close it. However, since it is already written, I figured it is worth at least discussing.
Concept ACK. I think that much more closely matches the semantics we want: extracting the bottom 64 bits of a 128 bits number as a signed integer, when the value is not in $[-2^{63},2^{63})$, is mathematically nonsense.
Also happy to rebase #1156 on top of this (it’ll simplify things a bit there too).
(uint64_t)(...) and (int64_t)(...) cast operations, which are distinct.
int64_t $h$ and uint64_t $l$
58@@ -59,7 +59,12 @@ static SECP256K1_INLINE void secp256k1_i128_rshift(secp256k1_int128 *r, unsigned
59 *r >>= n;
60 }
61
62+static SECP256K1_INLINE uint64_t secp256k1_i128_to_u64(const secp256k1_int128 *a) {
63+ return *a;
nit, just for clarity:
0 return (uint64_t)*a;
160+}
161+
162 static SECP256K1_INLINE int64_t secp256k1_i128_to_i64(const secp256k1_int128 *a) {
163- return (int64_t)a->lo;
164+ /* Verify that a represents a 64 bit signed value by checking that the high bits are a sign extension of the low bits. */
165+ VERIFY_CHECK(a->hi == -(uint64_t)(0x8000000000000000u == (0x8000000000000000u & a->lo)));
0 VERIFY_CHECK(a->hi == -(a->lo >> 63));
I find this easier to read (but it may just be me)
M62 to unsigned also in secp256k1_modinv64_normalize_62, and then let the VERIFY part of that function deal with the consequences of this. (It feels cleaner to optimize the cleanness of the real code instead of the test code.)
M62 is also used in r0 &= M62. Are you sure you want to get into the logic of mixing signed and unsigned bitwise operations?
/* test secp256k1_i128_to_i64 */ section to tests.c.