j: add avxSupport option #87546
j: add avxSupport option #87546
Conversation
|
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 | |||
There was a problem hiding this comment.
Not all builders might support avx, does compilation fails if that was the case? Does the binary run on machines without avx?
There was a problem hiding this comment.
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.
|
@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: 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...
I see some contradictory reports, including reports like that https://www.eurogamer.net/articles/digitalfoundry-2017-intel-pentium-g4560-budget-cpu-king-review_1 where people (who allegedely benchmarked the physical CPU in question) say that a new Intel Pentium CPU in the end of 2017 lacked AVX2, while not saying anything about AVX…
|
|
Oh interesting! Updated the PR to default to |
|
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.
Or at least better documented… slightly contradictory reports, slightly unclear Intel documentation…
|
Adds an
avxSupportoption to the J package. For now I've set this to default tofalsesince 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
falseand 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 = falseinstead of having to override the build step.Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)