-
Notifications
You must be signed in to change notification settings - Fork 17.6k
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
x/crypto/blake2b: amd64 SIMD assembly implementions not selected #24828
Comments
CC @tklauser My apologies, I didn't really think this CL through. We do need to keep x/crypto working well with older releases. I think we're going to need to use build tags here. |
chacha20poly1305 and the Argon2-AVX2 CL use custom CPU feature detection. Since Maybe group CPU feature detection as an internal package to avoid repeating code in |
Sorry, I was being too careless with https://golang.org/cl/106336 (and the original https://golang.org/cl/106235, I wasn't considering that anything outside of Go itself would depend on the So I think CPU feature detection as an internal package in |
While we wait for This is right now causing degraded performance for all code which is directly building binaries by |
Change https://golang.org/cl/108795 mentions this issue: |
… support" This reverts commit d644981 because of golang/go#24828. Change-Id: I14b4e1265b2e75897fa12548dbdfb77c308afaaf Reviewed-on: https://go-review.googlesource.com/108795 Reviewed-by: Tobias Klauser <tobias.klauser@gmail.com> Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
The build still seems to be broken after https://golang.org/cl/108795; see https://build.golang.org/?repo=golang.org%2fx%2fcrypto. Is there something more we need to revert? |
What kind of timeframe are we looking at for that? We shouldn't leave Should we roll back something from the |
We could revert https://golang.org/cl/106595 in the meantime and re-submit once #24843 is fixed and x/crypto has switched to x/sys/cpu. |
...and the same for https://golang.org/cl/106235 |
Why not just add the following (rough sketch) to the runtime as a temporary shim? // This is a temporary shim for x/crypto until issue 24843 is resolved.
var (
supports_avx = cpu.X86.HasAVX
supports_avx2 = cpu.X86.HasAVX2
) It can then be yanked out in go1.11 or go1.12 whenever #24843 is solved. |
Change https://golang.org/cl/110016 mentions this issue: |
I guess this is not needed anymore - since #24843 is fixed and CLs are merged. |
Fixed by golang/crypto@ae8bce0. |
What version of Go are you using (
go version
)?go 1.10.1
Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (
go env
)?What did you do?
go get golang.org/x/crypto
go test -v golang.org/x/crypto/blake2b
What did you expect to see?
What did you see instead?
CL 106336 switched from
runtime.support_avx()
(which was removed from the runtime) to a linker-mapping scheme. However it seems like the linker does not set the variables correctly. Furthermore the SSE4 var seems to use the wrong link-name:s / cpu.X86.HasSSE4 / cpu.X86.HasSSE41 See https://github.com/golang/go/blob/master/src/internal/cpu/cpu.go#L31
Even fixing this seems not to change anything. Further the the AVX and AVX2 code is not selected anyway.
/cc @ianlancetaylor
The text was updated successfully, but these errors were encountered: