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

amd-blis: 2.2 -> 3.0 #116499

Merged
merged 1 commit into from Mar 25, 2021
Merged

amd-blis: 2.2 -> 3.0 #116499

merged 1 commit into from Mar 25, 2021

Conversation

danieldk
Copy link
Contributor

Motivation for this change

Changelog:

https://github.com/amd/blis/releases/tag/3.0

Checked with the mt-dgemm/sgemm benchmark that there were no
regressions. Use through the BLAS alternative also works correctly.

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.

@r-rmcgibbo
Copy link

Result of nixpkgs-review pr 116499 at 3bc3465b run on x86_64-linux 1

2 packages built successfully:

@SuperSandro2000
Copy link
Member

This is a semi-automatic executed nixpkgs-review with nixpkgs-review-checks extension. It is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch).
If you have any questions or problems please reach out to SuperSandro2000 on IRC.

Result of nixpkgs-review pr 116499 run on x86_64-linux 1

2 packages failed to build and already failed to build on hydra master:

@danieldk
Copy link
Contributor Author

2 packages failed to build and already failed to build on hydra master:

@SuperSandro2000 no issues here. What CPU are the builds run on? Or of it's a VM, what SIMD features/architecture level is made available to the VM?

@SuperSandro2000
Copy link
Member

@SuperSandro2000 no issues here. What CPU are the builds run on? Or of it's a VM, what SIMD features/architecture level is made available to the VM?

tried both locally on a i7-4790 and a remote builder which I don't have the information about simd.

@danieldk
Copy link
Contributor Author

tried both locally on a i7-4790 and a remote builder which I don't have the information about simd.

Thanks for the info. This seems to be a bug in the generic gemmt kernel, which your machine falls back to because it does not have an AMD Zen CPU. However, I don't think it should be relevant for this package. This is AMD's blis fork updated for AMD Zen CPUs, there is really no point in running it on non-AMD CPUs (MKL would be the far superior choice, followed by OpenBLAS and upstream BLIS). On Zen CPUs a Zen-optimized kernel is used which passes the tests.

My only concern is that this could fail on Intel and pre-Zen Hydra nodes. So, one option would be to disable tests. (This is yet another example where we need to be able to select Hydra build machines based on machine properties.) We could also leave the tests in and accept Hydra failure. This is a leaf package that Zen users can use through the BLAS alternatives mechanism and it is not very expensive to build.

@SuperSandro2000
Copy link
Member

Or third option hydra does not build it at all.

@danieldk
Copy link
Contributor Author

Or third option hydra does not build it at all.

Or fourth: detect in the sandbox whether the builder has a Zen CPU and only run tests if it does. I think most builders don't, but that would at least make it easy to run the tests locally.

@danieldk
Copy link
Contributor Author

I have disabled tests for now and added a comment. We can try to re-enable them for the next release.

@SuperSandro2000 SuperSandro2000 merged commit 499600b into NixOS:master Mar 25, 2021
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

3 participants