LibCrypto: Fix up stuff from the SHA PR I can't live with #24668
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
👀 pr-needs-review
DanShaders
force-pushed
the
sha-actually-okay
branch
from
ee3a6d9
to
7670fcf
Compare
Hendiadyoin1
reviewed
Jul 5, 2024
MarekKnapek
reviewed
Jul 5, 2024
DanShaders
force-pushed
the
sha-actually-okay
branch
2 times, most recently
from
5c7b3e8
to
e5edeb1
Compare
nico
reviewed
Jul 6, 2024
There was a problem hiding this comment.
First three commits are fine.
From what I understand, the motivation for the 4th commit is to make the "gcc and clang currently can't do sha resolvers, so we have to do that manually" bug reports into a general framework, which a) seems like a somewhat questionable motivation to me b) I don't love the implementation, see below.
As always, it's well possible I'm misunderstanding things though!
DanShaders
force-pushed
the
sha-actually-okay
branch
2 times, most recently
from
21f3149
to
27b93f4
Compare
This warning is triggered when one accepts or returns vectors from a function (that is not marked with [[gnu::target(...)]]) which would have been otherwise passed in register if the current translation unit had been compiled with more permissive flags wrt instruction selection (i. e. if one adds -mavx2 to cmdline). This will never be a problem for us since we (a) never use different instruction selection options across ABI boundaries; (b) most of the affected functions are actually TU-local. Moreover, even if we somehow properly annotated all of the SIMD helpers, calling them across ABI (or target) boundaries would still be very dangerous because of inconsistent and bogus handling of [[gnu::target(...)]] across compilers. See llvm/llvm-project#64706 and https://www.reddit.com/r/cpp/comments/17qowl2/comment/k8j2odi .
This necessitates marking bit_cast as ALWAYS_INLINE since emitting it as a function call there will create an unnecessary potential SSE registers -> plain registers/memory round-trip.
This also removes `[[gnu::target("sse4.2")]]` from nested functions:
since we don't explicitly use any of the SSE4.2 intrinsics there, the
said target is unneeded. Note that this won't prevent compiler from
choosing SSE4.2 intrinsics as the affected functions are always inlined
into `transform_impl_sha1` that has `[[gnu::target("sse4.2")]]`.
These replace `#if ARCH(...)` macros that were added to conditionally include different hand-vectorized SIMD-implementations.
The helper doesn't use __builtin_cpu_supports (and instead makes raw cpuid calls) because of three reasons: - __builtin_cpu_supports only works on x86_64, so its usage need to be guarded with the preprocessor similarly to the current code. Moreover, we will have to use custom mechanisms to detect features on ARM, since there isn't such thing as cpuid there (and __builtin_cpu_* are not provided). - __builtin_cpu_supports doesn't support "sha" feature on all targeted toolchains currently. - And, of course, NIH.
Note: AVX target clone does not bring any significant (>0.5%) performance change.
It turns out we cannot use function multi-versioning with "sha" feature or even just plain ifunc resolvers without preprocessor guards. So, instead of feeding ifdef-soup monster, we just use static member function pointer. Moving the kernel into the SHA1 class makes it possible to not pass class members as parameters to it. This, however, requires us to disambiguate different target "clones" of the kernel using some kind of template.
See previous commit for rationale.
These do not bring any noticeable (>0.5%) performance improvements.
DanShaders
force-pushed
the
sha-actually-okay
branch
from
27b93f4
to
828ece1
Compare
| CircularBuffer.cpp | ||
| ConstrainedStream.cpp | ||
| CountingStream.cpp | ||
| DOSPackedTime.cpp | ||
| DeprecatedFlyString.cpp | ||
| ByteString.cpp |
There was a problem hiding this comment.
How did that get there
There was a problem hiding this comment.
This is called "expand selection to parenthesis", "sort lines".
There was a problem hiding this comment.
Maybe cross-check/coalesce with the Kernel CPUID file
There was a problem hiding this comment.
I think this will be a bit too much for a file than should be gone when fmv eventually matures.
|
Haven't reviewed in detail, but this fixes the current build failures on Alpine Linux, since ifunc is not supported there. |
github-actions
bot
removed
the
👀 pr-needs-review
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.