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

nauty: disable cpu feature detection #58961

Merged
merged 3 commits into from Apr 5, 2019
Merged

Conversation

timokau
Copy link
Member

@timokau timokau commented Apr 4, 2019

Motivation for this change

Nautys feature sniffing introduces bugs on older machines (like the hydra build machine on hydra). popcnt seems particularly problematic, I don't know how common it is for clz to be missing.

See https://groups.google.com/forum/#!msg/sage-packaging/Pe4SRDNYlhA/5_k9dZAoBAAJ.

Thanks @samueldr for hunting this issue down. CC @7c6f434c

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@timokau
Copy link
Member Author

timokau commented Apr 4, 2019

I'm not entirely sure if clz is based on a cpu feature. nauty's configure script checks for a cpu feature and for gcc support for popcnt but only for gcc support for clz.

If anyone know more here we may be able to get away with only disabling popcnt.

@timokau
Copy link
Member Author

timokau commented Apr 4, 2019

At least this page lists a popcnt feature but no clz. Could still be that gcc's builtin_clz is based on some other cpu feature, as https://stackoverflow.com/questions/9353973/implementation-of-builtin-clz seems to suggest.

@timokau timokau changed the title Nauty features nauty: disable cpu feature detection Apr 4, 2019
@7c6f434c
Copy link
Member

7c6f434c commented Apr 5, 2019

Wikipedia says that all AMD CPUs from the latest decade (2007 for first supporting CPUs) seems to support both popcnt and clz/lzcnt; Intel has started supporting clz/lzcnt only in 2013.

@timokau
Copy link
Member Author

timokau commented Apr 5, 2019

Thanks for looking into that. I'm still a bit confused as to weather or not gcc has software fallbacks at runtime when those instructions are missing, but at least for popcnt it apparently hasn't.

@timokau timokau merged commit 6662fbc into NixOS:master Apr 5, 2019
@timokau timokau deleted the nauty-features branch April 5, 2019 13:12
@timokau timokau added the 8.has: port to stable A PR already has a backport to the stable release. label Apr 5, 2019
@edolstra
Copy link
Member

edolstra commented Apr 5, 2019

We should probably specify somewhere what the minimum required CPU instruction set for Nixpkgs is. Not being able to use instructions like popcnt is pretty bad for performance.

@samueldr
Copy link
Member

samueldr commented Apr 5, 2019

@edolstra (this probably needs to be split off in a tracking issue) there's also the issue of the build farm having machines not having that instruction, while the others too. If the minimum supported features are defined, the build farm will need to be adjusted accordingly.

The solution might be to default to performant builds, and the package requiring a requiredSystemFeature of something like cpu-popcnt, every machines tagged accordingly. An end-user requiring this to be run on platforms missing the feature could override the package as needed.

And a question: would it be helpful if all cpu features were made available as a system feature within nix, maybe automatically "cpu:${feature}", meaning that it's not a configurable variable, but a runtime variable; no need to add new features to blacklist for reproducibility / perfs purposes, only require the right one. (Though it'd need policing in Nixpkgs to not abusively tag all features as required when not needed).

I'm thinking we already have similar issues in aarch64-land, where I've had binaries failing to run on Cortex-A53, being built on a Cortex-A96; though rebuilding locally it worked. (Don't remember what exactly.) As far as reproducibility goes, I don't know how ready we are with facing CPU features detection. I can't find anything quickly about that on the reproducible builds' website.

So uh, recap of the issues:

  • Some software detects CPU features.
  • Our build farm is heterogenous CPU-wise.

And things that aren't issues:

  • Things are being run on mis-matched CPUs with differing feature sets.

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

Successfully merging this pull request may close these issues.

None yet

5 participants