Skip to content

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

Merged
laanwj merged 1 commit intobitcoin:masterfrom
theuni:configure-check-asm
Jul 14, 2017
Merged

build: verify that the assembler can handle crc32 functions#10806
laanwj merged 1 commit intobitcoin:masterfrom
theuni:configure-check-asm

Conversation

@theuni
Copy link
Member

@theuni 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
gmaxwell approved these changes Jul 12, 2017
View reviewed changes
Copy link
Contributor

@gmaxwell gmaxwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@sipa
Copy link
Member

sipa commented Jul 12, 2017

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

@theuni
Copy link
Member Author

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.

@laanwj
Copy link
Member

laanwj commented Jul 13, 2017

Going to test.

@laanwj
Copy link
Member

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.

@theuni
Copy link
Member Author

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.

@laanwj
Copy link
Member

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 force-pushed the configure-check-asm branch from 393708c to d34d77a Compare July 13, 2017 16:48
@theuni
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.
@theuni
Copy link
Member Author

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.

@laanwj
Copy link
Member

laanwj commented Jul 14, 2017

Tested ACK d34d77a

@laanwj laanwj merged commit d34d77a into bitcoin:master Jul 14, 2017
laanwj added a commit that referenced this pull request Jul 14, 2017
@laanwj
Merge #10806: build: verify that the assembler can handle crc32 funct…
db825d2 
…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
@laanwj laanwj mentioned this pull request Jul 17, 2017
Closed
sickpig referenced this pull request in sickpig/BitcoinUnlimited Aug 17, 2017
@laanwj @sickpig
Port of Core's #10806: build: verify that the assembler can handle cr…
2da7406 
…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.
@sickpig sickpig mentioned this pull request Aug 17, 2017
Merged
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 18, 2019
@laanwj @PastaPastaPasta
Merge bitcoin#10806: build: verify that the assembler can handle crc3…
a0ff957 
…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
@str4d str4d mentioned this pull request Sep 22, 2020
Merged
zkbot added a commit to zcash/zcash that referenced this pull request Sep 25, 2020
@zkbot
Auto merge of #4739 - str4d:update-leveldb, r=str4d
514d868 
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.
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@gmaxwell gmaxwell gmaxwell approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Build system

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@theuni @sipa @laanwj @gmaxwell @fanquake