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.

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

CPU auto-detection feature / cfg gate #529

Closed
pinkforest opened this issue Jun 1, 2023 · 7 comments
Closed

CPU auto-detection feature / cfg gate #529

pinkforest opened this issue Jun 1, 2023 · 7 comments

Comments

@pinkforest
Copy link
Contributor

pinkforest commented Jun 1, 2023

This should not block landing #523 and can be done as follow-up

Follow-up from

Cc/ @jrose-signal @koute

@pinkforest
Copy link
Contributor Author

pinkforest commented Jun 1, 2023

Some q's

  1. Which use-cases would need to disable the CPU detecion ?
    1.1) Or is this just in case something goes wrong a.k.a "break glass" ?

  2. Is there much use for disabling CPU detection in granular basis
    2.1) Or should there be just plain override vs disallowing detection one-by-one ?

  3. Should we just re-use cfg(curve25519_dalek_backend) "override"
    3.1) which could be simd | simd_avx512 | simd_avx2

  4. We already use some hazardous / risky features that should be enabled with care and
    4.1) not sure how many (would) use --all-features

  5. If consensus is to do feature-set here then should the existing cfg(*_dalek_*) also migrated as features

Having a plain override with cfg() would align to everything else - it would be great to know what use-cases there are to make it easier if it's widely expected to be used ?

@jrose-signal
Copy link
Contributor

One use case for us (Signal) is code size, though not on Intel platforms. If there's ever autodetection for mobile platforms, we might want to turn it off if it's not enough of a speedup to be worth the extra code.

@tarcieri
Copy link
Contributor

tarcieri commented Jun 1, 2023

I would think that specifying curve25519_dalek_backend would override autodetection

@tarcieri
Copy link
Contributor

tarcieri commented Jun 1, 2023

If there's ever autodetection for mobile platforms, we might want to turn it off if it's not enough of a speedup to be worth the extra code.

I would think mobile platforms are ARM64, in which case the neon target feature is enabled by default (#457)

@pinkforest
Copy link
Contributor Author

Yeah consensus here seems to be that override seems to be the best.

There is this discussion re-opened so let's redirect there to simplify things further:

@tarcieri
Copy link
Contributor

Oh sorry, I forgot you opened this issue, but thanks for directing the conversation back to #414

@pinkforest
Copy link
Contributor Author

pinkforest commented Jun 12, 2023

Yeah I think we got consensus on this issue so your get-together words it well what needs ot happen overall :)

I mainly tried to gather use-cases to surface where additional complexity would be required but I don't think we have any.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants