Repository: dalek-cryptography/curve25519-dalek (language: rust)

Issue:
Experimental supports cfg
Re:
- https://github.com/dalek-cryptography/curve25519-dalek/issues/635
- https://github.com/dalek-cryptography/curve25519-dalek/issues/693

p.s. how do we implement the opt-in given we have declared backend details as implementation detail.

feature will not do as it poses problems of a) relaying across dependency chain b) not having top-level binary in control

Same as #414 we could use cfg for this - I  propose:

`cfg(curve25519_dalek_unstable = "avx512")`

Which will in turn support for unstable AVX512 through nightly opt-in unstables basis.



Discussion / hints:
I would suggest using the existing `curve25519_dalek_backend`cfg rather than adding yet another one
Problem is this is more like sub-configuration and it conflates configuration between stable and unstable configuration.

By denoting explicitly that it's unstable, it avoids the confusion of opting into something that is not entirely stable.

e.g. curve25519_dalek_backend is mostly for stable backend configuration and is provided for solely as "override"

Also backend is more backend class and not explicit backend, e.g. "simd" can mean anything in between those stable-only backends.

Additionally providing override to sub-backend gets confusing when it's supposed to be only override and not opt-in cfg.
Having two different cfg options for the backend reintroduces the problem that they can both be set in conflicting states, which was the whole reason we moved away from features in the first place.

I think we already have all the cfg options we need and the main thing we need to do at this point is *not* automatically enable nightly-only features.
The cfg conflict happens even today, it is perfectly possible to set two cfg values stuffed into same key -

However we guard and test that in build script just like we would guard the opt-in which is different to override.

Adding it under _backend will complicate the gating and will pollute unstable configuration where it has = "simd"

Logically speaking if we have `curve25519_dalek_backend = "simd"` and then we have `"avx512"` it will be confusing?

When somebody writes `--cfg curve25519_dalek_backend="simd" --cfg curve25519_dalek_backend="avx512" `

It gets confusing right? because the above would work .. and confusingly people might confuse it as stable backend

We could say `--cfg curve25519_dalek_backend="unstable_avx512"` perhaps but still confusing (less though) IMO
Your example would be caught by the compiler, as opposed to requiring logic at every backend selection site.

I very strongly believe we should not (re)introduce backend selection knobs that duplicate/conflict with each other.
The current backend knobs are overrides and it does not guarantee selection though.

We can settle with `backend="unstable_avx512"` and at least the value denotes the override into unstable sub-backend.

btw compiler doesn't mind duplicate --cfg values and it's something I had to address earlier
Regardless, the point is not to have logic in backend selection code all over the place that has to handle potentially conflicting user inputs.

That would be a major regression over where we are now.
Wanna comment on having the unstable_ in the value to denote it's unstable-ness? That would allow it in one cfg.
That seems better than two potentially conflicting attributes
Cool great let's do that

EDIT: Another could have been `curve25519_dalek_backend = "simd_unstable_avx512"` but it's longer w/o value

Fix patch:
diff --git a/curve25519-dalek/CHANGELOG.md b/curve25519-dalek/CHANGELOG.md
index 939d3bfba..619aa32ca 100644
--- a/curve25519-dalek/CHANGELOG.md
+++ b/curve25519-dalek/CHANGELOG.md
@@ -3,6 +3,10 @@
 Entries are listed in reverse chronological order per undeprecated
 major series.
 
+## Unreleased
+
+* Move AVX-512 backend selection logic to a separate CFG flag that requires nightly
+
 ## 4.x series
 
 ### 4.1.3
diff --git a/curve25519-dalek/Cargo.toml b/curve25519-dalek/Cargo.toml
index c4fb28a38..fc35264bb 100644
--- a/curve25519-dalek/Cargo.toml
+++ b/curve25519-dalek/Cargo.toml
@@ -76,7 +76,7 @@ curve25519-dalek-derive = { version = "0.1", path = "../curve25519-dalek-derive"
 level = "warn"
 check-cfg = [
     'cfg(allow_unused_unsafe)',
-    'cfg(curve25519_dalek_backend, values("fiat", "serial", "simd"))',
+    'cfg(curve25519_dalek_backend, values("fiat", "serial", "simd", "unstable_avx512"))',
     'cfg(curve25519_dalek_diagnostics, values("build"))',
     'cfg(curve25519_dalek_bits, values("32", "64"))',
     'cfg(nightly)',
diff --git a/curve25519-dalek/README.md b/curve25519-dalek/README.md
index c918d6959..e85163636 100644
--- a/curve25519-dalek/README.md
+++ b/curve25519-dalek/README.md
@@ -90,11 +90,12 @@ This release also does a lot of dependency updates and relaxations to unblock up
 
 Curve arithmetic is implemented and used by one of the following backends:
 
-| Backend  | Selection | Implementation                                                | Bits / Word sizes |
-| :---     | :---      | :---                                                          | :---              |
-| `serial` | Automatic | An optimized, non-parllel implementation                      | `32` and `64`     |
-| `fiat`   | Manual    | Formally verified field arithmetic from [fiat-crypto]         | `32` and `64`     |
-| `simd`   | Automatic | Intel AVX2 / AVX512 IFMA accelerated backend                  | `64` only         |
+| Backend           | Selection | Implementation                                           | Bits / Word sizes |
+| :---              | :---      | :---                                                     | :---              |
+| `serial`          | Automatic | An optimized, non-parllel implementation                 | `32` and `64`     |
+| `fiat`            | Manual    | Formally verified field arithmetic from [fiat-crypto]    | `32` and `64`     |
+| `simd`            | Automatic | Intel AVX2 accelerated backend                           | `64` only         |
+| `unstable_avx512` | Manual    | Intel AVX512 IFMA accelerated backend (requires nightly) | `64` only         |
 
 At runtime, `curve25519-dalek` selects an arithmetic backend from the set of backends it was compiled to support. For Intel x86-64 targets, unless otherwise specified, it will build itself with `simd` support, and default to `serial` at runtime if the appropriate CPU features aren't detected. See [SIMD backend] for more details.
 
@@ -148,16 +149,16 @@ $ cargo build --target i686-unknown-linux-gnu
 
 ## SIMD backend
 
-The specific SIMD backend (AVX512 / AVX2 / `serial` default) is selected automatically at runtime, depending on the currently available CPU features, and whether Rust nightly is being used for compilation. The precise conditions are specified below.
+When the `simd` backend is selected, the AVX2 or `serial` implementation is selected automatically at runtime, depending on the currently available CPU features. Similarly, when the `unstable_avx512` backend is selected, the AVX512 implementation is selected automatically at runtime if available, or else selection falls through to the aforementioned `simd` backend logic.
 
 For a given CPU feature, you can also specify an appropriate `-C target_feature` to build a binary which assumes the required SIMD instructions are always available. Don't do this if you don't have a good reason.
 
 | Backend | `RUSTFLAGS`                               | Requires nightly? |
 | :---    | :---                                      | :---              |
-| avx2    | `-C target_feature=+avx2`                 | no                |
-| avx512  | `-C target_feature=+avx512ifma,+avx512vl` | yes               |
+| AVX2    | `-C target_feature=+avx2`                 | no                |
+| AVX512  | `-C target_feature=+avx512ifma,+avx512vl` | yes               |
 
-If compiled on a non-nightly compiler, `curve25519-dalek` will not include AVX512 code, and therefore will never select it at runtime.
+To reiterate, the `simd` backend will NOT use AVX512 code under any circumstance. The only way to enable AVX512 currently is to select the `unstable_avx512` backend and use a nightly compiler.
 
 # Documentation
 
diff --git a/curve25519-dalek/build.rs b/curve25519-dalek/build.rs
index 97fa28524..1b0f618a6 100644
--- a/curve25519-dalek/build.rs
+++ b/curve25519-dalek/build.rs
@@ -35,13 +35,16 @@ fn main() {
 
     println!("cargo:rustc-cfg=curve25519_dalek_bits=\"{curve25519_dalek_bits}\"");
 
-    if rustc_version::version_meta()
+    let nightly = if rustc_version::version_meta()
         .expect("failed to detect rustc version")
         .channel
         == rustc_version::Channel::Nightly
     {
         println!("cargo:rustc-cfg=nightly");
-    }
+        true
+    } else {
+        false
+    };
 
     let rustc_version = rustc_version::version().expect("failed to detect rustc version");
     if rustc_version.major == 1 && rustc_version.minor <= 64 {
@@ -51,25 +54,44 @@ fn main() {
     }
 
     // Backend overrides / defaults
-    let curve25519_dalek_backend =
-        match std::env::var("CARGO_CFG_CURVE25519_DALEK_BACKEND").as_deref() {
-            Ok("fiat") => "fiat",
-            Ok("serial") => "serial",
-            Ok("simd") => {
-                // simd can only be enabled on x86_64 & 64bit target_pointer_width
-                match is_capable_simd(&target_arch, curve25519_dalek_bits) {
-                    true => "simd",
-                    // If override is not possible this must result to compile error
-                    // See: issues/532
-                    false => panic!("Could not override curve25519_dalek_backend to simd"),
+    let curve25519_dalek_backend = match std::env::var("CARGO_CFG_CURVE25519_DALEK_BACKEND")
+        .as_deref()
+    {
+        Ok("fiat") => "fiat",
+        Ok("serial") => "serial",
+        Ok("simd") => {
+            // simd can only be enabled on x86_64 & 64bit target_pointer_width
+            match is_capable_simd(&target_arch, curve25519_dalek_bits) {
+                true => "simd",
+                // If override is not possible this must result to compile error
+                // See: issues/532
+                false => panic!("Could not override curve25519_dalek_backend to simd"),
+            }
+        }
+        Ok("unstable_avx512") if nightly => {
+            // simd can only be enabled on x86_64 & 64bit target_pointer_width
+            match is_capable_simd(&target_arch, curve25519_dalek_bits) {
+                true => {
+                    // In addition enable Avx2 fallback through simd stable backend
+                    // NOTE: Compiler permits duplicate / multi value on the same key
+                    println!("cargo:rustc-cfg=curve25519_dalek_backend=\"simd\"");
+
+                    "unstable_avx512"
                 }
+                // If override is not possible this must result to compile error
+                // See: issues/532
+                false => panic!("Could not override curve25519_dalek_backend to unstable_avx512"),
             }
-            // default between serial / simd (if potentially capable)
-            _ => match is_capable_simd(&target_arch, curve25519_dalek_bits) {
-                true => "simd",
-                false => "serial",
-            },
-        };
+        }
+        Ok("unstable_avx512") if !nightly => {
+            panic!("Could not override curve25519_dalek_backend to unstable_avx512, as this is nigthly only");
+        }
+        // default between serial / simd (if potentially capable)
+        _ => match is_capable_simd(&target_arch, curve25519_dalek_bits) {
+            true => "simd",
+            false => "serial",
+        },
+    };
     println!("cargo:rustc-cfg=curve25519_dalek_backend=\"{curve25519_dalek_backend}\"");
 }
 
diff --git a/curve25519-dalek/src/backend/mod.rs b/curve25519-dalek/src/backend/mod.rs
index 9ad1dd3de..c5fab97f5 100644
--- a/curve25519-dalek/src/backend/mod.rs
+++ b/curve25519-dalek/src/backend/mod.rs
@@ -46,14 +46,14 @@ pub mod vector;
 enum BackendKind {
     #[cfg(curve25519_dalek_backend = "simd")]
     Avx2,
-    #[cfg(all(curve25519_dalek_backend = "simd", nightly))]
+    #[cfg(all(curve25519_dalek_backend = "unstable_avx512", nightly))]
     Avx512,
     Serial,
 }
 
 #[inline]
 fn get_selected_backend() -> BackendKind {
-    #[cfg(all(curve25519_dalek_backend = "simd", nightly))]
+    #[cfg(all(curve25519_dalek_backend = "unstable_avx512", nightly))]
     {
         cpufeatures::new!(cpuid_avx512, "avx512ifma", "avx512vl");
         let token_avx512: cpuid_avx512::InitToken = cpuid_avx512::init();
@@ -88,7 +88,7 @@ where
         #[cfg(curve25519_dalek_backend = "simd")]
         BackendKind::Avx2 =>
             vector::scalar_mul::pippenger::spec_avx2::Pippenger::optional_multiscalar_mul::<I, J>(scalars, points),
-        #[cfg(all(curve25519_dalek_backend = "simd", nightly))]
+        #[cfg(all(curve25519_dalek_backend = "unstable_avx512", nightly))]
         BackendKind::Avx512 =>
             vector::scalar_mul::pippenger::spec_avx512ifma_avx512vl::Pippenger::optional_multiscalar_mul::<I, J>(scalars, points),
         BackendKind::Serial =>
@@ -100,7 +100,7 @@ where
 pub(crate) enum VartimePrecomputedStraus {
     #[cfg(curve25519_dalek_backend = "simd")]
     Avx2(vector::scalar_mul::precomputed_straus::spec_avx2::VartimePrecomputedStraus),
-    #[cfg(all(curve25519_dalek_backend = "simd", nightly))]
+    #[cfg(all(curve25519_dalek_backend = "unstable_avx512", nightly))]
     Avx512ifma(
         vector::scalar_mul::precomputed_straus::spec_avx512ifma_avx512vl::VartimePrecomputedStraus,
     ),
@@ -120,7 +120,7 @@ impl VartimePrecomputedStraus {
             #[cfg(curve25519_dalek_backend = "simd")]
             BackendKind::Avx2 =>
                 VartimePrecomputedStraus::Avx2(vector::scalar_mul::precomputed_straus::spec_avx2::VartimePrecomputedStraus::new(static_points)),
-            #[cfg(all(curve25519_dalek_backend = "simd", nightly))]
+            #[cfg(all(curve25519_dalek_backend = "unstable_avx512", nightly))]
             BackendKind::Avx512 =>
                 VartimePrecomputedStraus::Avx512ifma(vector::scalar_mul::precomputed_straus::spec_avx512ifma_avx512vl::VartimePrecomputedStraus::new(static_points)),
             BackendKind::Serial =>
@@ -150,7 +150,7 @@ impl VartimePrecomputedStraus {
                 dynamic_scalars,
                 dynamic_points,
             ),
-            #[cfg(all(curve25519_dalek_backend = "simd", nightly))]
+            #[cfg(all(curve25519_dalek_backend = "unstable_avx512", nightly))]
             VartimePrecomputedStraus::Avx512ifma(inner) => inner.optional_mixed_multiscalar_mul(
                 static_scalars,
                 dynamic_scalars,
@@ -181,7 +181,7 @@ where
         BackendKind::Avx2 => {
             vector::scalar_mul::straus::spec_avx2::Straus::multiscalar_mul::<I, J>(scalars, points)
         }
-        #[cfg(all(curve25519_dalek_backend = "simd", nightly))]
+        #[cfg(all(curve25519_dalek_backend = "unstable_avx512", nightly))]
         BackendKind::Avx512 => {
             vector::scalar_mul::straus::spec_avx512ifma_avx512vl::Straus::multiscalar_mul::<I, J>(
                 scalars, points,
@@ -210,7 +210,7 @@ where
                 scalars, points,
             )
         }
-        #[cfg(all(curve25519_dalek_backend = "simd", nightly))]
+        #[cfg(all(curve25519_dalek_backend = "unstable_avx512", nightly))]
         BackendKind::Avx512 => {
             vector::scalar_mul::straus::spec_avx512ifma_avx512vl::Straus::optional_multiscalar_mul::<
                 I,
@@ -228,7 +228,7 @@ pub fn variable_base_mul(point: &EdwardsPoint, scalar: &Scalar) -> EdwardsPoint
     match get_selected_backend() {
         #[cfg(curve25519_dalek_backend = "simd")]
         BackendKind::Avx2 => vector::scalar_mul::variable_base::spec_avx2::mul(point, scalar),
-        #[cfg(all(curve25519_dalek_backend = "simd", nightly))]
+        #[cfg(all(curve25519_dalek_backend = "unstable_avx512", nightly))]
         BackendKind::Avx512 => {
             vector::scalar_mul::variable_base::spec_avx512ifma_avx512vl::mul(point, scalar)
         }
@@ -242,7 +242,7 @@ pub fn vartime_double_base_mul(a: &Scalar, A: &EdwardsPoint, b: &Scalar) -> Edwa
     match get_selected_backend() {
         #[cfg(curve25519_dalek_backend = "simd")]
         BackendKind::Avx2 => vector::scalar_mul::vartime_double_base::spec_avx2::mul(a, A, b),
-        #[cfg(all(curve25519_dalek_backend = "simd", nightly))]
+        #[cfg(all(curve25519_dalek_backend = "unstable_avx512", nightly))]
         BackendKind::Avx512 => {
             vector::scalar_mul::vartime_double_base::spec_avx512ifma_avx512vl::mul(a, A, b)
         }
diff --git a/curve25519-dalek/src/backend/vector/mod.rs b/curve25519-dalek/src/backend/vector/mod.rs
index 2839dca45..3a9104412 100644
--- a/curve25519-dalek/src/backend/vector/mod.rs
+++ b/curve25519-dalek/src/backend/vector/mod.rs
@@ -16,7 +16,7 @@ pub mod packed_simd;
 
 pub mod avx2;
 
-#[cfg(nightly)]
+#[cfg(all(curve25519_dalek_backend = "unstable_avx512", nightly))]
 pub mod ifma;
 
 pub mod scalar_mul;
diff --git a/curve25519-dalek/src/backend/vector/scalar_mul/pippenger.rs b/curve25519-dalek/src/backend/vector/scalar_mul/pippenger.rs
index 1376c4eab..ed89cdefe 100644
--- a/curve25519-dalek/src/backend/vector/scalar_mul/pippenger.rs
+++ b/curve25519-dalek/src/backend/vector/scalar_mul/pippenger.rs
@@ -11,7 +11,10 @@
 
 #[curve25519_dalek_derive::unsafe_target_feature_specialize(
     "avx2",
-    conditional("avx512ifma,avx512vl", nightly)
+    conditional(
+        "avx512ifma,avx512vl",
+        all(curve25519_dalek_backend = "unstable_avx512", nightly)
+    )
 )]
 pub mod spec {
 
diff --git a/curve25519-dalek/src/backend/vector/scalar_mul/precomputed_straus.rs b/curve25519-dalek/src/backend/vector/scalar_mul/precomputed_straus.rs
index 1f16ab3e1..6038d18c7 100644
--- a/curve25519-dalek/src/backend/vector/scalar_mul/precomputed_straus.rs
+++ b/curve25519-dalek/src/backend/vector/scalar_mul/precomputed_straus.rs
@@ -13,7 +13,10 @@
 
 #[curve25519_dalek_derive::unsafe_target_feature_specialize(
     "avx2",
-    conditional("avx512ifma,avx512vl", nightly)
+    conditional(
+        "avx512ifma,avx512vl",
+        all(curve25519_dalek_backend = "unstable_avx512", nightly)
+    )
 )]
 pub mod spec {
 
diff --git a/curve25519-dalek/src/backend/vector/scalar_mul/straus.rs b/curve25519-dalek/src/backend/vector/scalar_mul/straus.rs
index 413e6fd9a..70e4949db 100644
--- a/curve25519-dalek/src/backend/vector/scalar_mul/straus.rs
+++ b/curve25519-dalek/src/backend/vector/scalar_mul/straus.rs
@@ -13,7 +13,10 @@
 
 #[curve25519_dalek_derive::unsafe_target_feature_specialize(
     "avx2",
-    conditional("avx512ifma,avx512vl", nightly)
+    conditional(
+        "avx512ifma,avx512vl",
+        all(curve25519_dalek_backend = "unstable_avx512", nightly)
+    )
 )]
 pub mod spec {
 
diff --git a/curve25519-dalek/src/backend/vector/scalar_mul/variable_base.rs b/curve25519-dalek/src/backend/vector/scalar_mul/variable_base.rs
index 9f924f286..585945209 100644
--- a/curve25519-dalek/src/backend/vector/scalar_mul/variable_base.rs
+++ b/curve25519-dalek/src/backend/vector/scalar_mul/variable_base.rs
@@ -2,7 +2,10 @@
 
 #[curve25519_dalek_derive::unsafe_target_feature_specialize(
     "avx2",
-    conditional("avx512ifma,avx512vl", nightly)
+    conditional(
+        "avx512ifma,avx512vl",
+        all(curve25519_dalek_backend = "unstable_avx512", nightly)
+    )
 )]
 pub mod spec {
 
diff --git a/curve25519-dalek/src/backend/vector/scalar_mul/vartime_double_base.rs b/curve25519-dalek/src/backend/vector/scalar_mul/vartime_double_base.rs
index ea2af8ad4..64254e248 100644
--- a/curve25519-dalek/src/backend/vector/scalar_mul/vartime_double_base.rs
+++ b/curve25519-dalek/src/backend/vector/scalar_mul/vartime_double_base.rs
@@ -13,7 +13,10 @@
 
 #[curve25519_dalek_derive::unsafe_target_feature_specialize(
     "avx2",
-    conditional("avx512ifma,avx512vl", nightly)
+    conditional(
+        "avx512ifma,avx512vl",
+        all(curve25519_dalek_backend = "unstable_avx512", nightly)
+    )
 )]
 pub mod spec {
 


Test patch: