build: verify that the assembler can handle crc32 functions #10806

Merged
merged 1 commit into from Jul 14, 2017

Conversation

Projects
None yet
5 participants
Member

theuni commented Jul 12, 2017

Also, enable crc32 even if -msse4.2 wasn't added by us, as long as it works. This allows custom flags (such as -march=native) to work as expected.

Addresses #10670.

@gmaxwell

utACK

Owner

sipa commented Jul 12, 2017

@theuni I'm confused why the old code wouldn't build the sse library with the correct flags?

Member

theuni commented Jul 13, 2017

@sipa This addresses 2 things:

  1. Disables crc32 on platforms where the assembler can't handle it
  2. Enables crc32 if cflags/cxxflags are overridden but the intrinsics still work.

I think 2 is what you're asking about? If so, it wouldn't work before because, even though the user has specified -msse4.2 (or -march=native, or whatever it takes to get the intrinsics working), our build wouldn't have the necessary make option set here: https://github.com/bitcoin/bitcoin/blob/master/src/Makefile.leveldb.include#L145, so the necessary define wouldn't be added.

Owner

laanwj commented Jul 13, 2017

Going to test.

Owner

laanwj commented Jul 13, 2017

Interesting. On OpenBSD 6.1:

configure:18285: checking whether C++ compiler accepts -msse4.2  
configure:18304: eg++ -std=c++11 -c -g -O2 -Wall -Wextra -Wformat -Wvla -Wformat-security -Wno-unused-parameter -Werror -msse4.2  conftest.cpp >&5
configure:18304: $? = 0
configure:18313: result: yes
configure:18326: checking for assembler crc32 support  
configure:18351: eg++ -std=c++11 -c -g -O2 -Wall -Wextra -Wformat -Wvla -Wformat-security -Wno-unused-parameter -msse4.2  conftest.cpp >&5
conftest.cpp: In function 'int main()':
conftest.cpp:34:14: warning: variable 'l' set but not used [-Wunused-but-set-variable]
     uint64_t l = 0;
              ^
configure:18351: $? = 0
configure:18352: result: yes
configure:20040: checking for pk

Then when running gmake:

leveldb/port/port_posix_sse.cc: In function 'uint32_t leveldb::port::AcceleratedCRC32C(uint32_t, const char*, size_t)':
leveldb/port/port_posix_sse.cc:59:15: warning: 'ecx' may be used uninitialized in this function [-Wmaybe-uninitialized]
   return (ecx & (1 << 20)) != 0;
               ^
leveldb/port/port_posix_sse.cc:57:26: note: 'ecx' was declared here
   unsigned int eax, ebx, ecx, edx;
                          ^
/tmp//cc5NhjN4.s: Assembler messages:
/tmp//cc5NhjN4.s:95: Error: no such instruction: `crc32b -1(%rbp),%eax'
/tmp//cc5NhjN4.s:121: Error: no such instruction: `crc32q -8(%rbp),%rax'
/tmp//cc5NhjN4.s:148: Error: no such instruction: `crc32b -1(%rbp),%eax'
:312: Error: no such instruction: `crc32l 0(%rbp),%eax'
gmake[2]: *** [Makefile:4923: leveldb/port/leveldb_libleveldb_sse42_a-port_posix_sse.o] Error 1

So somehow it works while running configure, but not while building confused.

Member

theuni commented Jul 13, 2017

@laanwj Grr, I checked an asm dump of the test to make sure that the crc32s are actually emitted, but it looks like I forgot to test with -O2. Indeed with optims on, they're optimized out. I'll add something to use the result.

Owner

laanwj commented Jul 13, 2017

@theuni this solves it:

diff --git a/configure.ac b/configure.ac
index 5c3dac648..6b5d891fa 100644
--- a/configure.ac
+++ b/configure.ac
@@ -262,9 +262,10 @@ AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[
     #endif
   ]],[[
     uint64_t l = 0;
-    l = _mm_crc32_u8(0, 0);
-    l = _mm_crc32_u32(0, 0);
-    l = _mm_crc32_u64(0, 0);
+    l = _mm_crc32_u8(l, 0);
+    l = _mm_crc32_u32(l, 0);
+    l = _mm_crc32_u64(l, 0);
+    return l;
   ]])],
  [ AC_MSG_RESULT(yes); enable_hwcrc32=yes],
  [ AC_MSG_RESULT(no)]
@theuni theuni build: verify that the assembler can handle crc32 functions
Also, enable crc32 even if -msse4.2 wasn't added by us, as long as it works.
This allows custom flags (such as -march=native) to work as expected.
d34d77a
Member

theuni commented Jul 13, 2017

@laanwj Aha. I was messing with printf to force it to stay around, I didn't realize that the test function was "int main()". That makes total sense and is really good to know!

Pushed with your fix.

Owner

laanwj commented Jul 14, 2017

Tested ACK d34d77a

@laanwj laanwj merged commit d34d77a into bitcoin:master Jul 14, 2017

1 check passed

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

@laanwj laanwj added a commit that referenced this pull request Jul 14, 2017

@laanwj laanwj Merge #10806: build: verify that the assembler can handle crc32 funct…
…ions


d34d77a build: verify that the assembler can handle crc32 functions (Cory Fields)

Pull request description:

  Also, enable crc32 even if -msse4.2 wasn't added by us, as long as it works. This allows custom flags (such as -march=native) to work as expected.

  Addresses #10670.

Tree-SHA512: e1a41a87b078d270bc645814315b229ad9c16556a4d14fb66b27a65b28d0caf9bf324f8c1e221854992aa17f53466eece06faebbf74d59b3d4ff2e6db6c614a4
db825d2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment