For AVX2 code, also check for AVX, XSAVE, and OS support #13471

pull sipa wants to merge 1 commits into bitcoin:master from sipa:201806_avxossupport changing 1 files +17 −4

sipa commented at 5:45 PM on June 14, 2018: member

Fixes #12903.
laanwj added the label Bug on Jun 14, 2018
DrahtBot commented at 8:07 PM on June 14, 2018: member
Note to reviewers: This pull request conflicts with the following ones:
- #13386 (SHA256 implementations based on Intel SHA Extensions by sipa)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
in src/crypto/sha256.cpp:492 in 14a2ccccbc outdated
```
 483 | @@ -484,6 +484,14 @@ void inline cpuid(uint32_t leaf, uint32_t subleaf, uint32_t& a, uint32_t& b, uin
 484 |  {
 485 |    __asm__ ("cpuid" : "=a"(a), "=b"(b), "=c"(c), "=d"(d) : "0"(leaf), "2"(subleaf));
 486 |  }
 487 | +
 488 | +/** Check whether the OS has enabled AVX registers. */
 489 | +bool AVXEnabled()
 490 | +{
 491 | +    uint32_t a, d;
 492 | +    __asm__("xgetbv" : "=a"(a), "=d"(d) : "c"(0));
```
Empact commented at 9:10 PM on June 14, 2018:

Possible to use _xgetbv intrinsic here? https://software.intel.com/en-us/node/694243

sipa commented at 9:11 PM on June 14, 2018:

I tried that, but it doesn't seem GCC has that.
Empact commented at 9:14 PM on June 14, 2018:

How about something like this?:

#ifndef _xgetbv static inline unsigned long long _xgetbv(unsigned int index){ unsigned int eax, edx; __asm__ __volatile__("xgetbv" : "=a"(eax), "=d"(edx) : "c"(index)); return ((unsigned long long)edx << 32) | eax; } #endif

https://lists.macports.org/pipermail/macports-dev/2013-January/021782.html
sipa commented at 9:21 PM on June 14, 2018:

It's the same thing, just specialized for AVX (which AFAIK is the only thing it's useful for entirely).

ken2812221 commented at 9:25 PM on June 14, 2018:

From gcc code I don't think _xgetbv is a mcro https://github.com/gcc-mirror/gcc/commit/154452f45aad4bfe7a1f719c65cd26837f416a48#diff-547c091edc6345b286585818a4ae7528R60
sipa commented at 10:41 PM on June 14, 2018:

Access to _xgetbv requires compiling with -mxsave, and you can't run any -mxsave code without verifying you're running on a CPU which supports XSAVE. There are a few options around this:

Add a separate libbitcoin_crypto_xsave.a library, with just the AVXEnabled() function (seems a lot of effort)

Put AVXEnabled() in libbitcoin_crypto_avx2.a (which would then be compiled with -mavx2 -mxsave). Downside is that we'd need to put it in all AVX libraries (there may be a -mavx and/or -mbmi2 library too later).

Compile everything with -mxsave (when supported by the compiler) but just don't call _xgetbv without first checking (which in theory might be unsafe as the compiler could emit xgetbv instructions elsewhere).

I think all 3 are worse than 1 line of inline asm.
Empact commented at 11:18 PM on June 14, 2018:

Agreed
in src/crypto/sha256.cpp:491 in 14a2ccccbc outdated
```
 483 | @@ -484,6 +484,14 @@ void inline cpuid(uint32_t leaf, uint32_t subleaf, uint32_t& a, uint32_t& b, uin
 484 |  {
 485 |    __asm__ ("cpuid" : "=a"(a), "=b"(b), "=c"(c), "=d"(d) : "0"(leaf), "2"(subleaf));
 486 |  }
 487 | +
 488 | +/** Check whether the OS has enabled AVX registers. */
 489 | +bool AVXEnabled()
 490 | +{
 491 | +    uint32_t a, d;
```
theuni commented at 9:14 PM on June 15, 2018:

Does this need #ifdef USE_ASM ? I'm really not even sure what the option is intended to mean anymore.

sipa commented at 11:24 PM on June 15, 2018:

It's within a #if defined(USE_ASM) && (defined(__x86_64__) || defined(__amd64__) || defined(__i386__)) block.

theuni commented at 12:04 AM on June 16, 2018:

Indeed, nevermind.
theuni commented at 9:15 PM on June 15, 2018: member

utACK 14a2ccccbc27bd136005c8b057809c8663418e4a
in src/crypto/sha256.cpp:503 in 14a2ccccbc outdated
```
 499 | @@ -492,6 +500,7 @@ std::string SHA256AutoDetect()
 500 |  {
 501 |      std::string ret = "standard";
 502 |  #if defined(USE_ASM) && (defined(__x86_64__) || defined(__amd64__) || defined(__i386__))
 503 | +    (void)AVXEnabled;
```
Empact commented at 8:55 PM on June 17, 2018:

nit: Presuming this line is to avoid an unused function warning, how about instead either:

Commenting as to its purpose

Defining it in an #else block below

Only defining AVXEnabled when defined(ENABLE_AVX2) && !defined(BUILD_BITCOIN_INTERNAL)?
sipa commented at 9:57 PM on June 18, 2018:

I added a comment.

I'd rather not add logic around the compilation of the function itself, as it may grow increasingly complex in the future (if we also have avx1 code, for example). It's in an anonymous namespace anyway, so if it's unused, the compiler will drop it.
For AVX2 code, also check for AVX, XSAVE, and OS support 32d153fa36

in src/crypto/sha256.cpp:489 in 14a2ccccbc outdated

 483 | @@ -484,6 +484,14 @@ void inline cpuid(uint32_t leaf, uint32_t subleaf, uint32_t& a, uint32_t& b, uin
 484 |  {
 485 |    __asm__ ("cpuid" : "=a"(a), "=b"(b), "=c"(c), "=d"(d) : "0"(leaf), "2"(subleaf));
 486 |  }
 487 | +
 488 | +/** Check whether the OS has enabled AVX registers. */
 489 | +bool AVXEnabled()

laanwj commented at 3:45 PM on June 18, 2018:

Could be static

sipa commented at 3:49 PM on June 18, 2018:

It's inside an anonymous namespace.

sipa force-pushed on Jun 18, 2018
Empact commented at 1:05 AM on June 19, 2018: member

utACK 32d153f - note I did not separately research and validate the bit checking
laanwj merged this on Jun 24, 2018
laanwj closed this on Jun 24, 2018
laanwj referenced this in commit 5eca4e86d4 on Jun 24, 2018
codablock referenced this in commit 5e6ee8ea5d on Oct 1, 2019
codablock referenced this in commit d07f54b967 on Oct 1, 2019
barrystyle referenced this in commit ff8dc4902f on Jan 22, 2020
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)