Merge #793: Make scalar/field choice depend on C-detected __int128 availability

79f1f7a4f1 Autodetect __int128 availability on the C side (Pieter Wuille)
0d7727f95e Add SECP256K1_FE_STORAGE_CONST_GET to 5x52 field (Pieter Wuille)

Pull request description:

  This PR does two things:
  * It removes the ability to select the 5x52 field with a 8x32 scalar, or the 10x26 field with a 4x64 scalar. It's both 128-bit wide versions, or neither.
  * The choice is made automatically by the C code, unless overridden by a USE_FORCE_WIDEMUL_INT{64,128} define (which is available through `configure` with a hidden option --with-test-override-wide-multiplication={auto,int64,int128}).

  This reduces the reliance on autoconf for this performance-critical configuration option, and also reduces the number of different combinations to test.

  This removes one theoretically useful combination: if you had x86_64 asm but no __int128 support in your compiler, it was possible to use the 64-bit field before but the 32-bit scalar. I think this doesn't matter as all compilers/systems that support (our)  x86_64 asm also support __int128. Furthermore, #767 will break this.

  As an unexpected side effect, this also means the `gen_context` static precomputation tool will now use __int128 based implementations when available (which required an addition to the 5x52 field; see first commit).

ACKs for top commit:
  real-or-random:
    ACK 79f1f7a4f1 diff looks good and tests pass
  elichai:
    tACK  79f1f7a4f1

Tree-SHA512: 4171732668e5c9cae5230e3a43dd6df195567e1232b89c12c5db429986b6519bb4d77334cb0bac8ce13a00a24dfffdff69b46c89b4d59bc6d297a996ea4efd3d

.travis.yml

@@ -17,17 +17,17 @@ compiler:
   - gcc
 env:
   global:
-    - FIELD=auto  BIGNUM=auto  SCALAR=auto  ENDOMORPHISM=no  STATICPRECOMPUTATION=yes  ECMULTGENPRECISION=auto  ASM=no  BUILD=check  EXTRAFLAGS=  HOST=  ECDH=no  RECOVERY=no  EXPERIMENTAL=no CTIMETEST=yes BENCH=yes ITERS=2
+    - WIDEMUL=auto  BIGNUM=auto  ENDOMORPHISM=no  STATICPRECOMPUTATION=yes  ECMULTGENPRECISION=auto  ASM=no  BUILD=check  EXTRAFLAGS=  HOST=  ECDH=no  RECOVERY=no  EXPERIMENTAL=no CTIMETEST=yes BENCH=yes ITERS=2
   matrix:
-    - SCALAR=32bit    RECOVERY=yes
+    - WIDEMUL=int64   RECOVERY=yes
-    - SCALAR=32bit    FIELD=32bit       ECDH=yes  EXPERIMENTAL=yes
+    - WIDEMUL=int64   ECDH=yes  EXPERIMENTAL=yes
-    - SCALAR=64bit
+    - WIDEMUL=int64   ENDOMORPHISM=yes
-    - FIELD=64bit     RECOVERY=yes
+    - WIDEMUL=int128
-    - FIELD=64bit     ENDOMORPHISM=yes
+    - WIDEMUL=int128  RECOVERY=yes
-    - FIELD=64bit     ENDOMORPHISM=yes  ECDH=yes EXPERIMENTAL=yes
+    - WIDEMUL=int128  ENDOMORPHISM=yes
-    - FIELD=64bit                       ASM=x86_64
+    - WIDEMUL=int128  ENDOMORPHISM=yes  ECDH=yes EXPERIMENTAL=yes
-    - FIELD=64bit     ENDOMORPHISM=yes  ASM=x86_64
+    - WIDEMUL=int128                    ASM=x86_64
-    - FIELD=32bit     ENDOMORPHISM=yes
+    - WIDEMUL=int128  ENDOMORPHISM=yes  ASM=x86_64
     - BIGNUM=no
     - BIGNUM=no       ENDOMORPHISM=yes RECOVERY=yes EXPERIMENTAL=yes
     - BIGNUM=no       STATICPRECOMPUTATION=no

build-aux/m4/bitcoin_secp.m4

@@ -1,8 +1,3 @@
-dnl libsecp25k1 helper checks
-AC_DEFUN([SECP_INT128_CHECK],[
-has_int128=$ac_cv_type___int128
-])
-
 dnl escape "$0x" below using the m4 quadrigaph @S|@, and escape it again with a \ for the shell.
 AC_DEFUN([SECP_64BIT_ASM_CHECK],[
 AC_MSG_CHECKING(for x86_64 assembly availability)

configure.ac

@@ -141,15 +141,13 @@ AC_ARG_ENABLE(external_default_callbacks,
     [use_external_default_callbacks=$enableval],
     [use_external_default_callbacks=no])
 
-AC_ARG_WITH([field], [AS_HELP_STRING([--with-field=64bit|32bit|auto],
+dnl Test-only override of the (autodetected by the C code) "widemul" setting.
-[finite field implementation to use [default=auto]])],[req_field=$withval], [req_field=auto])
+dnl Legal values are int64 (for [u]int64_t), int128 (for [unsigned] __int128), and auto (the default).
+AC_ARG_WITH([test-override-wide-multiply], [] ,[set_widemul=$withval], [set_widemul=auto])
 
 AC_ARG_WITH([bignum], [AS_HELP_STRING([--with-bignum=gmp|no|auto],
 [bignum implementation to use [default=auto]])],[req_bignum=$withval], [req_bignum=auto])
 
-AC_ARG_WITH([scalar], [AS_HELP_STRING([--with-scalar=64bit|32bit|auto],
-[scalar implementation to use [default=auto]])],[req_scalar=$withval], [req_scalar=auto])
-
 AC_ARG_WITH([asm], [AS_HELP_STRING([--with-asm=x86_64|arm|no|auto],
 [assembly optimizations to use (experimental: arm) [default=auto]])],[req_asm=$withval], [req_asm=auto])
 
@@ -170,8 +168,6 @@ AC_ARG_WITH([ecmult-gen-precision], [AS_HELP_STRING([--with-ecmult-gen-precision
 )],
 [req_ecmult_gen_precision=$withval], [req_ecmult_gen_precision=auto])
 
-AC_CHECK_TYPES([__int128])
-
 AC_CHECK_HEADER([valgrind/memcheck.h], [enable_valgrind=yes], [enable_valgrind=no], [])
 AM_CONDITIONAL([VALGRIND_ENABLED],[test "$enable_valgrind" = "yes"])
 
@@ -265,63 +261,6 @@ else
   esac
 fi
 
-if test x"$req_field" = x"auto"; then
-  if test x"set_asm" = x"x86_64"; then
-    set_field=64bit
-  fi
-  if test x"$set_field" = x; then
-    SECP_INT128_CHECK
-    if test x"$has_int128" = x"yes"; then
-      set_field=64bit
-    fi
-  fi
-  if test x"$set_field" = x; then
-    set_field=32bit
-  fi
-else
-  set_field=$req_field
-  case $set_field in
-  64bit)
-    if test x"$set_asm" != x"x86_64"; then
-      SECP_INT128_CHECK
-      if test x"$has_int128" != x"yes"; then
-        AC_MSG_ERROR([64bit field explicitly requested but neither __int128 support or x86_64 assembly available])
-      fi
-    fi
-    ;;
-  32bit)
-    ;;
-  *)
-    AC_MSG_ERROR([invalid field implementation selection])
-    ;;
-  esac
-fi
-
-if test x"$req_scalar" = x"auto"; then
-  SECP_INT128_CHECK
-  if test x"$has_int128" = x"yes"; then
-    set_scalar=64bit
-  fi
-  if test x"$set_scalar" = x; then
-    set_scalar=32bit
-  fi
-else
-  set_scalar=$req_scalar
-  case $set_scalar in
-  64bit)
-    SECP_INT128_CHECK
-    if test x"$has_int128" != x"yes"; then
-      AC_MSG_ERROR([64bit scalar explicitly requested but __int128 support not available])
-    fi
-    ;;
-  32bit)
-    ;;
-  *)
-    AC_MSG_ERROR([invalid scalar implementation selected])
-    ;;
-  esac
-fi
-
 if test x"$req_bignum" = x"auto"; then
   SECP_GMP_CHECK
   if test x"$has_gmp" = x"yes"; then
@@ -365,16 +304,18 @@ no)
   ;;
 esac
 
-# select field implementation
+# select wide multiplication implementation
-case $set_field in
+case $set_widemul in
-64bit)
+int128)
-  AC_DEFINE(USE_FIELD_5X52, 1, [Define this symbol to use the FIELD_5X52 implementation])
+  AC_DEFINE(USE_FORCE_WIDEMUL_INT128, 1, [Define this symbol to force the use of the (unsigned) __int128 based wide multiplication implementation])
   ;;
-32bit)
+int64)
-  AC_DEFINE(USE_FIELD_10X26, 1, [Define this symbol to use the FIELD_10X26 implementation])
+  AC_DEFINE(USE_FORCE_WIDEMUL_INT64, 1, [Define this symbol to force the use of the (u)int64_t based wide multiplication implementation])
+  ;;
+auto)
   ;;
 *)
-  AC_MSG_ERROR([invalid field implementation])
+  AC_MSG_ERROR([invalid wide multiplication implementation])
   ;;
 esac
 
@@ -396,19 +337,6 @@ no)
   ;;
 esac
 
-#select scalar implementation
-case $set_scalar in
-64bit)
-  AC_DEFINE(USE_SCALAR_4X64, 1, [Define this symbol to use the 4x64 scalar implementation])
-  ;;
-32bit)
-  AC_DEFINE(USE_SCALAR_8X32, 1, [Define this symbol to use the 8x32 scalar implementation])
-  ;;
-*)
-  AC_MSG_ERROR([invalid scalar implementation])
-  ;;
-esac
-
 #set ecmult window size
 if test x"$req_ecmult_window" = x"auto"; then
   set_ecmult_window=15
@@ -553,10 +481,12 @@ echo "  module recovery         = $enable_module_recovery"
 echo
 echo "  asm                     = $set_asm"
 echo "  bignum                  = $set_bignum"
-echo "  field                   = $set_field"
-echo "  scalar                  = $set_scalar"
 echo "  ecmult window size      = $set_ecmult_window"
 echo "  ecmult gen prec. bits   = $set_ecmult_gen_precision"
+dnl Hide test-only options unless they're used.
+if test x"$set_widemul" != xauto; then
+echo "  wide multiplication     = $set_widemul"
+fi
 echo
 echo "  valgrind                = $enable_valgrind"
 echo "  CC                      = $CC"

contrib/travis.sh

@@ -14,7 +14,7 @@ fi
 
 ./configure \
     --enable-experimental="$EXPERIMENTAL" --enable-endomorphism="$ENDOMORPHISM" \
-    --with-field="$FIELD" --with-bignum="$BIGNUM" --with-asm="$ASM" --with-scalar="$SCALAR" \
+    --with-test-override-wide-multiply="$WIDEMUL" --with-bignum="$BIGNUM" --with-asm="$ASM" \
     --enable-ecmult-static-precomputation="$STATICPRECOMPUTATION" --with-ecmult-gen-precision="$ECMULTGENPRECISION" \
     --enable-module-ecdh="$ECDH" --enable-module-recovery="$RECOVERY" \
     --host="$HOST" $EXTRAFLAGS

src/basic-config.h

@@ -14,24 +14,20 @@
 #undef USE_ENDOMORPHISM
 #undef USE_EXTERNAL_ASM
 #undef USE_EXTERNAL_DEFAULT_CALLBACKS
-#undef USE_FIELD_10X26
-#undef USE_FIELD_5X52
 #undef USE_FIELD_INV_BUILTIN
 #undef USE_FIELD_INV_NUM
 #undef USE_NUM_GMP
 #undef USE_NUM_NONE
-#undef USE_SCALAR_4X64
-#undef USE_SCALAR_8X32
 #undef USE_SCALAR_INV_BUILTIN
 #undef USE_SCALAR_INV_NUM
+#undef USE_FORCE_WIDEMUL_INT64
+#undef USE_FORCE_WIDEMUL_INT128
 #undef ECMULT_WINDOW_SIZE
-#undef HAVE___INT128 /* used in util.h */
 
 #define USE_NUM_NONE 1
 #define USE_FIELD_INV_BUILTIN 1
 #define USE_SCALAR_INV_BUILTIN 1
-#define USE_FIELD_10X26 1
+#define USE_WIDEMUL_64 1
-#define USE_SCALAR_8X32 1
 #define ECMULT_WINDOW_SIZE 15
 
 #endif /* USE_BASIC_CONFIG */

src/field.h

@@ -22,16 +22,16 @@
 #include "libsecp256k1-config.h"
 #endif
 
-#if defined(USE_FIELD_10X26)
-#include "field_10x26.h"
-#elif defined(USE_FIELD_5X52)
-#include "field_5x52.h"
-#else
-#error "Please select field implementation"
-#endif
-
 #include "util.h"
 
+#if defined(SECP256K1_WIDEMUL_INT128)
+#include "field_5x52.h"
+#elif defined(SECP256K1_WIDEMUL_INT64)
+#include "field_10x26.h"
+#else
+#error "Please select wide multiplication implementation"
+#endif
+
 /** Normalize a field element. This brings the field element to a canonical representation, reduces
  *  its magnitude to 1, and reduces it modulo field size `p`.
  */

src/field_5x52.h

@@ -46,4 +46,10 @@ typedef struct {
     (d6) | (((uint64_t)(d7)) << 32) \
 }}
 
+#define SECP256K1_FE_STORAGE_CONST_GET(d) \
+    (uint32_t)(d.n[3] >> 32), (uint32_t)d.n[3], \
+    (uint32_t)(d.n[2] >> 32), (uint32_t)d.n[2], \
+    (uint32_t)(d.n[1] >> 32), (uint32_t)d.n[1], \
+    (uint32_t)(d.n[0] >> 32), (uint32_t)d.n[0]
+
 #endif /* SECP256K1_FIELD_REPR_H */

src/field_impl.h

@@ -14,12 +14,12 @@
 #include "util.h"
 #include "num.h"
 
-#if defined(USE_FIELD_10X26)
+#if defined(SECP256K1_WIDEMUL_INT128)
-#include "field_10x26_impl.h"
-#elif defined(USE_FIELD_5X52)
 #include "field_5x52_impl.h"
+#elif defined(SECP256K1_WIDEMUL_INT64)
+#include "field_10x26_impl.h"
 #else
-#error "Please select field implementation"
+#error "Please select wide multiplication implementation"
 #endif
 
 SECP256K1_INLINE static int secp256k1_fe_equal(const secp256k1_fe *a, const secp256k1_fe *b) {

src/scalar.h

@@ -8,6 +8,7 @@
 #define SECP256K1_SCALAR_H
 
 #include "num.h"
+#include "util.h"
 
 #if defined HAVE_CONFIG_H
 #include "libsecp256k1-config.h"
@@ -15,12 +16,12 @@
 
 #if defined(EXHAUSTIVE_TEST_ORDER)
 #include "scalar_low.h"
-#elif defined(USE_SCALAR_4X64)
+#elif defined(SECP256K1_WIDEMUL_INT128)
 #include "scalar_4x64.h"
-#elif defined(USE_SCALAR_8X32)
+#elif defined(SECP256K1_WIDEMUL_INT64)
 #include "scalar_8x32.h"
 #else
-#error "Please select scalar implementation"
+#error "Please select wide multiplication implementation"
 #endif
 
 /** Clear a scalar to prevent the leak of sensitive data. */

src/scalar_impl.h

@@ -16,12 +16,12 @@
 
 #if defined(EXHAUSTIVE_TEST_ORDER)
 #include "scalar_low_impl.h"
-#elif defined(USE_SCALAR_4X64)
+#elif defined(SECP256K1_WIDEMUL_INT128)
 #include "scalar_4x64_impl.h"
-#elif defined(USE_SCALAR_8X32)
+#elif defined(SECP256K1_WIDEMUL_INT64)
 #include "scalar_8x32_impl.h"
 #else
-#error "Please select scalar implementation"
+#error "Please select wide multiplication implementation"
 #endif
 
 static const secp256k1_scalar secp256k1_scalar_one = SECP256K1_SCALAR_CONST(0, 0, 0, 0, 0, 0, 0, 1);

src/util.h

@@ -170,13 +170,10 @@ static SECP256K1_INLINE void *manual_alloc(void** prealloc_ptr, size_t alloc_siz
 # define I64uFORMAT "llu"
 #endif
 
-#if defined(HAVE___INT128)
+#if defined(__GNUC__)
-# if defined(__GNUC__)
+# define SECP256K1_GNUC_EXT __extension__
-#  define SECP256K1_GNUC_EXT __extension__
+#else
-# else
+# define SECP256K1_GNUC_EXT
-#  define SECP256K1_GNUC_EXT
-# endif
-SECP256K1_GNUC_EXT typedef unsigned __int128 uint128_t;
 #endif
 
 /* Zero memory if flag == 1. Flag must be 0 or 1. Constant time. */
@@ -213,4 +210,20 @@ static SECP256K1_INLINE void secp256k1_int_cmov(int *r, const int *a, int flag)
     *r = (int)(r_masked | a_masked);
 }
 
+/* If USE_FORCE_WIDEMUL_{INT128,INT64} is set, use that wide multiplication implementation.
+ * Otherwise use the presence of __SIZEOF_INT128__ to decide.
+ */
+#if defined(USE_FORCE_WIDEMUL_INT128)
+# define SECP256K1_WIDEMUL_INT128 1
+#elif defined(USE_FORCE_WIDEMUL_INT64)
+# define SECP256K1_WIDEMUL_INT64 1
+#elif defined(__SIZEOF_INT128__)
+# define SECP256K1_WIDEMUL_INT128 1
+#else
+# define SECP256K1_WIDEMUL_INT64 1
+#endif
+#if defined(SECP256K1_WIDEMUL_INT128)
+SECP256K1_GNUC_EXT typedef unsigned __int128 uint128_t;
+#endif
+
 #endif /* SECP256K1_UTIL_H */