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

theuni · 2017-07-12T17:18:48Z

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

sipa · 2017-07-12T20:09:38Z

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

theuni · 2017-07-13T05:28:52Z

@sipa This addresses 2 things:

Disables crc32 on platforms where the assembler can't handle it
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.

laanwj · 2017-07-13T13:00:41Z

Going to test.

laanwj · 2017-07-13T15:32:32Z

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.

theuni · 2017-07-13T16:01:45Z

@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.

laanwj · 2017-07-13T16:10:22Z

@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)]

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.

theuni · 2017-07-13T16:49:15Z

@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.

laanwj · 2017-07-14T07:22:35Z

Tested ACK d34d77a

…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

…c32 functions 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.

…2 functions 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 bitcoin#10670. Tree-SHA512: e1a41a87b078d270bc645814315b229ad9c16556a4d14fb66b27a65b28d0caf9bf324f8c1e221854992aa17f53466eece06faebbf74d59b3d4ff2e6db6c614a4

Update LevelDB to upstream commit f545dfabf Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#7911 - bitcoin/bitcoin#7982 - bitcoin/bitcoin#8133 - bitcoin/bitcoin#8784 - Only the missing changes. - bitcoin/bitcoin#8826 - bitcoin/bitcoin#8613 - bitcoin/bitcoin#10544 - bitcoin/bitcoin#10633 - Only the changes to files and code we have. - bitcoin/bitcoin#10806 - bitcoin/bitcoin#10958 - bitcoin/bitcoin#12451 - bitcoin/bitcoin#13925 - bitcoin/bitcoin#15270 This upgrades LevelDB in the exact same commit progression as upstream, up to January 2019.

gmaxwell approved these changes Jul 12, 2017

View reviewed changes

fanquake added the Build system label Jul 12, 2017

theuni force-pushed the configure-check-asm branch from 393708c to d34d77a Compare July 13, 2017 16:48

build: verify that the assembler can handle crc32 functions

d34d77a

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.

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

laanwj mentioned this pull request Jul 17, 2017

Master doesn't build on OpenBSD (leveldb) #10670

Closed

sickpig mentioned this pull request Aug 17, 2017

[port] Further LevelDB ports BitcoinUnlimited/BitcoinUnlimited#755

Merged

str4d mentioned this pull request Sep 22, 2020

Update LevelDB to upstream commit f545dfabf zcash/zcash#4739

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

theuni commented Jul 12, 2017

gmaxwell left a comment

sipa commented Jul 12, 2017

theuni commented Jul 13, 2017

laanwj commented Jul 13, 2017

laanwj commented Jul 13, 2017

theuni commented Jul 13, 2017

laanwj commented Jul 13, 2017

theuni commented Jul 13, 2017

laanwj commented Jul 14, 2017

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

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

Conversation

theuni commented Jul 12, 2017

gmaxwell left a comment

Choose a reason for hiding this comment

sipa commented Jul 12, 2017

theuni commented Jul 13, 2017

laanwj commented Jul 13, 2017

laanwj commented Jul 13, 2017

theuni commented Jul 13, 2017

laanwj commented Jul 13, 2017

theuni commented Jul 13, 2017

laanwj commented Jul 14, 2017