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

j: add avxSupport option #87546

Merged
merged 1 commit into from May 13, 2020
Merged

j: add avxSupport option #87546

merged 1 commit into from May 13, 2020

Conversation

@FireyFly
Copy link
Contributor

FireyFly commented May 10, 2020

Adds an avxSupport option to the J package. For now I've set this to default to false since that matches the behaviour prior to this change, and J does seem like the kind of package where you might want AVX support out of the box.

Personally, I think I'd prefer defaulting to false and requiring opt-in for AVX support so that the default build works more widely across machines, but it's not really something I feel strongly about either way.

Motivation for this change

Package wouldn't build as-is on non-AVX x86-64 systems. As is currently, it still doesn't build out of the box, but you can at least straightforwardly set avxSupport = false instead of having to override the build step.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@7c6f434c
Copy link
Member

7c6f434c commented May 13, 2020

Hm, is it true that current-generation «Pentium» laptop CPUs lack AVX? If yes, I am definitely in favour of defaulting to no AVX.

@@ -1,4 +1,6 @@
{ stdenv, fetchFromGitHub, readline, libedit, bc }:
{ stdenv, fetchFromGitHub, readline, libedit, bc
, avxSupport ? true

This comment has been minimized.

Copy link
@Mic92

Mic92 May 13, 2020

Contributor

Not all builders might support avx, does compilation fails if that was the case? Does the binary run on machines without avx?

This comment has been minimized.

Copy link
@FireyFly

FireyFly May 13, 2020

Author Contributor

Ah, I should've clarified that. The package compiles fine without AVX support, but the testsuite fails since the resulting binary doesn't run on machines without AVX support. Running into that when trying to build the package was what prompted this PR in the first place.

To be clear though, the package in its current state in master already does require AVX support, so defaulting to true is the "no change to status quo, just make it configurable" option.

@FireyFly
Copy link
Contributor Author

FireyFly commented May 13, 2020

@7c6f434c: AVX seems to have been introduced in Intel CPUs with Sandy Bridge in 2011, and then later by AMD the same year. I suspect all current (and recent) laptops support AVX just fine...

To me, I feel it'd normally make sense to err on the side of "default to false" with flags like this to prioritise wider support by default--2011 isn't that long ago. The thing that makes me hesitate a bit is how J as a language is kind of specialised toward heavy array processing, and fits the niche where you want to make good use of SIMD extensions when possible.

@7c6f434c
Copy link
Member

7c6f434c commented May 13, 2020

@FireyFly FireyFly force-pushed the FireyFly:pkgs/j/avx-flag branch from 55ee6fe to 20235a8 May 13, 2020
@FireyFly
Copy link
Contributor Author

FireyFly commented May 13, 2020

Oh interesting! Updated the PR to default to false instead, since it seems we all feel that makes more sense as a default. It could always be changed later down the line once AVX support is more widespread.

@ofborg ofborg bot requested a review from 7c6f434c May 13, 2020
@7c6f434c
Copy link
Member

7c6f434c commented May 13, 2020

@7c6f434c 7c6f434c merged commit b2f83be into NixOS:master May 13, 2020
16 checks passed
16 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="20235a8"; rev="20235a89f8f2539c043a1820a686d9468df82032"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="20235a8"; rev="20235a89f8f2539c043a1820a686d9468df82032"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="20235a8"; rev="20235a89f8f2539c043a1820a686d9468df82032"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="20235a8"; rev="20235a89f8f2539c043a1820a686d9468df82032"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="20235a8"; rev="20235a89f8f2539c043a1820a686d9468df82032"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="20235a8"; rev="20235a89f8f2539c043a1820a686d9468df82032"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="20235a8"; rev="20235a89f8f2539c043a1820a686d9468df82032"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
j, j.passthru.tests on aarch64-linux Success
Details
j, j.passthru.tests on x86_64-linux Success
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.