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

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

Merged
merged 1 commit into from Jun 24, 2018

Conversation

Projects
None yet
6 participants
@sipa
Copy link
Member

sipa commented Jun 14, 2018

Fixes #12903.

@laanwj laanwj added the Bug label Jun 14, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Jun 14, 2018

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.

bool AVXEnabled()
{
uint32_t a, d;
__asm__("xgetbv" : "=a"(a), "=d"(d) : "c"(0));

This comment has been minimized.

@Empact

Empact Jun 14, 2018

Member

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

This comment has been minimized.

@sipa

sipa Jun 14, 2018

Author Member

I tried that, but it doesn't seem GCC has that.

This comment has been minimized.

@Empact

Empact Jun 14, 2018

Member

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

This comment has been minimized.

@sipa

sipa Jun 14, 2018

Author Member

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

This comment has been minimized.

@ken2812221

ken2812221 Jun 14, 2018

Member

From gcc code I don't think _xgetbv is a mcro
gcc-mirror/gcc@154452f#diff-547c091edc6345b286585818a4ae7528R60

This comment has been minimized.

@sipa

sipa Jun 14, 2018

Author Member

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.

This comment has been minimized.

@Empact

Empact Jun 14, 2018

Member

Agreed

/** Check whether the OS has enabled AVX registers. */
bool AVXEnabled()
{
uint32_t a, d;

This comment has been minimized.

@theuni

theuni Jun 15, 2018

Member

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

This comment has been minimized.

@sipa

sipa Jun 15, 2018

Author Member

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

This comment has been minimized.

@theuni

theuni Jun 16, 2018

Member

Indeed, nevermind.

@theuni

This comment has been minimized.

Copy link
Member

theuni commented Jun 15, 2018

utACK 14a2ccc

@@ -492,6 +500,7 @@ std::string SHA256AutoDetect()
{
std::string ret = "standard";
#if defined(USE_ASM) && (defined(__x86_64__) || defined(__amd64__) || defined(__i386__))
(void)AVXEnabled;

This comment has been minimized.

@Empact

Empact Jun 17, 2018

Member

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)?

This comment has been minimized.

@sipa

sipa Jun 18, 2018

Author Member

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.

@@ -484,6 +484,14 @@ void inline cpuid(uint32_t leaf, uint32_t subleaf, uint32_t& a, uint32_t& b, uin
{
__asm__ ("cpuid" : "=a"(a), "=b"(b), "=c"(c), "=d"(d) : "0"(leaf), "2"(subleaf));
}

/** Check whether the OS has enabled AVX registers. */
bool AVXEnabled()

This comment has been minimized.

@laanwj

laanwj Jun 18, 2018

Member

Could be static

This comment has been minimized.

@sipa

sipa Jun 18, 2018

Author Member

It's inside an anonymous namespace.

@sipa sipa force-pushed the sipa:201806_avxossupport branch from 14a2ccc to 32d153f Jun 18, 2018

@Empact

This comment has been minimized.

Copy link
Member

Empact commented Jun 19, 2018

utACK 32d153f - note I did not separately research and validate the bit checking

@laanwj laanwj merged commit 32d153f into bitcoin:master Jun 24, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jun 24, 2018

Merge #13471: For AVX2 code, also check for AVX, XSAVE, and OS support
32d153f For AVX2 code, also check for AVX, XSAVE, and OS support (Pieter Wuille)

Pull request description:

  Fixes #12903.

Tree-SHA512: 01e71efb5d3a43c49a145a5b1dc4fe7d0a491e1e78479e7df830a2aaac57c3dcfc316e28984c695206c76f93b68e4350fc037ca36756ca579b7070e39c835da2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.