build: Restrict check for CRC32C intrinsic to aarch64 #23045

laanwj · 2021-09-20T10:27:49Z

crc32c's hardware accelerated code doesn't handle ARM 32-bit at all, but only 64-bit. Make the check in configure.ac check for this architecture explicitly. This change does not affect non-ARM architectures.

For the release binaries, the current configure.ac check happens to work: it enables it on aarch64 but disables it for armhf. However some combination of compiler version and settings can cause this check to succeed on armhf (as reported on IRC). So make the 64-bit platform requirement explicit.

(details: while the check already explicitly checks the __crc32d intrinsic, which strictly doesn't exist on 32-bit ARM, this is not enough! gcc happens to helpfully emulate it:

These built-in intrinsics for the ARMv8-A CRC32 extension are available when the -march=armv8-a+crc switch is used "uint32_t __crc32d (uint32_t, uint64_t)" Form of expected instruction(s): Two crc32w r0, r0, r0 instructions for AArch32

)

laanwj · 2021-09-20T10:28:19Z

Thanks to @luke-jr for reporting this.

luke-jr · 2021-09-20T14:07:07Z

ARM "emulates" it with two crc32w from what you said on IRC. But is that actually slower than the fallback non-assembly code? If it performs better, maybe we'd be better off making it work (as it apparently did in 0.21)

luke-jr · 2021-09-20T14:07:45Z

configure.ac

nit: AArch64 is a more accurate name, since it's not really related to the 32-bit ARM architecture AIUI.

Sure, fine with changing the name, though what crc32c library uses internally is ARM64 so I mimiced that.

laanwj · 2021-09-20T14:53:12Z

Tested the GUIX build:

$ aarch64-linux-gnu-objdump -d 64/bitcoin-f6a7ed55c9bc/bin/bitcoind |grep crc32 | wc
    136     816    5341
$ arm-linux-gnueabihf-objdump -d 32/bitcoin-f6a7ed55c9bc/bin/bitcoind |grep crc32 | wc
      0       0       0

ARM "emulates" it with two crc32w from what you said on IRC. But is that actually slower than the fallback non-assembly code? If it performs better, maybe we'd be better off making it work (as it apparently did in 0.21)

No, it can never have worked like this. I'm obviously fine with someone adapting crc32c's hardware acceleration to work on 32-bit ARM, but be aware that 32-bit hardware with support for CRC32C is exceedingly rare. It's probably why Google didn't bother.

Until then this is the fix to make it at least not crash.

luke-jr · 2021-09-20T23:13:25Z

tACK f6a7ed55c9bc16fcf48d3d4b957170f12393049e (also ACK AArch64 rename variant thereof)

`crc32c`'s hardware accelerated code doesn't handle ARM 32-bit at all. Make the check in `configure.ac` check for this architecture explicitly. For the release binaries, the current `configure.ac` check happens to work: it enables it on aarch64 but disables it for armhf. However some combination of compiler version and settings might ostensibly cause this check to succeed on armhf (as reported on IRC). So make the 64-bit platform requirement explicit.

laanwj · 2021-09-21T10:38:25Z

Force pushed, only change ARM64→Aarch64
f6a7ed55c9bc16fcf48d3d4b957170f12393049e…f2747d1602ec4e1128356b861b2167daf66a845b

DrahtBot · 2021-09-22T01:19:35Z

Guix builds

File	commit `226731a` (master)	commit ee8c3a19260a07455aa7c6fcdabfc459f30e6b2e (master and this pull)
SHA256SUMS.part	`3b0fb952af02fea7...`	`12e6ec1bfbd4bb60...`
*-aarch64-linux-gnu-debug.tar.gz	`ebddd981105cda54...`	`29cee6f0fd831056...`
*-aarch64-linux-gnu.tar.gz	`06ed68405650507a...`	`43ca1309f2a661b7...`
*-arm-linux-gnueabihf-debug.tar.gz	`171f501c0774635d...`	`4721a4f9ed0bcd77...`
*-arm-linux-gnueabihf.tar.gz	`6d4934aa9df9d38f...`	`ea0154146a4a5faa...`
*-osx-unsigned.dmg	`e4263f82001b2a20...`	`52546c3edfac3147...`
*-osx-unsigned.tar.gz	`dcc64c104b66b92b...`	`db54064edb3980c3...`
*-osx64.tar.gz	`1ccdb0871e9ec7df...`	`567ca71dd6845046...`
*-powerpc64-linux-gnu-debug.tar.gz	`30ccf10d5d3f946c...`	`3b6821d55e6832e0...`
*-powerpc64-linux-gnu.tar.gz	`ae28a2385243126d...`	`936b440ac751070d...`
*-powerpc64le-linux-gnu-debug.tar.gz	`b6a28f795856c441...`	`4b8367c3aa9f8f7a...`
*-powerpc64le-linux-gnu.tar.gz	`17b6fc3521884b9c...`	`a6845022b493786e...`
*-riscv64-linux-gnu-debug.tar.gz	`80a16cb8b0d6b45d...`	`6c1cc3ec97baffc8...`
*-riscv64-linux-gnu.tar.gz	`d1d22dd4669869a0...`	`3cdd1d8b0f6aa119...`
*-win-unsigned.tar.gz	`42dcdb895a2b48ee...`	`68414f3889b0bffc...`
*-win64-debug.zip	`0ba1f6dbbfcd40e2...`	`e42f9037b72ac6dc...`
*-win64-setup-unsigned.exe	`764532404d55f7d9...`	`47f17865c19f905d...`
*-win64.zip	`4a57723d8a0c5684...`	`9c47c6bc39d3d56e...`
*-x86_64-linux-gnu-debug.tar.gz	`46f79071f4053088...`	`af58d5d12b7134d7...`
*-x86_64-linux-gnu.tar.gz	`aa2063637f637fcb...`	`cc0aca53afe108bf...`
*.tar.gz	`a8c6814a55f7445e...`	`ae4d27c4eb4e79c2...`
guix_build.log	`511b84519ce45cdd...`	`fa5684498ce1757c...`
guix_build.log.diff		`c495b251a38153df...`

luke-jr · 2021-09-22T13:56:12Z

re-tACK f2747d1

jarolrod

ACK f2747d1

Not to familiar with the inner workings here, but I read up a bit on this. Additionally, I ran a GUIX build successfully on this PR branch, and subsequently tested the ARM binary on a 32-bit beaglebone by running bitcoind.

GUIX hashes:

$ env HOSTS='arm-linux-gnueabihf aarch64-linux-gnu' ./contrib/guix/guix-build
$ find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum

5b1885691d142f4a37b5823380c5818428cf90182e30673c410c29edbcc137ca  guix-build-f2747d1602ec/output/aarch64-linux-gnu/SHA256SUMS.part
59e1cf3d773a4ea046ab4f6aec790c1d90a96ed2e70e9c2c7b8bd88d5dbeac75  guix-build-f2747d1602ec/output/aarch64-linux-gnu/bitcoin-f2747d1602ec-aarch64-linux-gnu-debug.tar.gz
b692531334d33cc356772b0813f4497880b00f978623a15919697c41e66c2b58  guix-build-f2747d1602ec/output/aarch64-linux-gnu/bitcoin-f2747d1602ec-aarch64-linux-gnu.tar.gz
61f403c56ee0bdeaeb7af8859057adae52c7acb02fd55c822c33ec186ea51880  guix-build-f2747d1602ec/output/arm-linux-gnueabihf/SHA256SUMS.part
7f87851a8c5adde3bdcdecbd9bcd1c6631540def1d616297a3c542373cb9a412  guix-build-f2747d1602ec/output/arm-linux-gnueabihf/bitcoin-f2747d1602ec-arm-linux-gnueabihf-debug.tar.gz
fdf5ae161a5314fe9119ebb49e9b2344be8b8cd7465a20047632b06419f5fa03  guix-build-f2747d1602ec/output/arm-linux-gnueabihf/bitcoin-f2747d1602ec-arm-linux-gnueabihf.tar.gz
fb6f2ebe3f30608c3e05baac3373aeea41032515cedc482b88efe0136377a901  guix-build-f2747d1602ec/output/dist-archive/bitcoin-f2747d1602ec.tar.gz

…rch64 f2747d1 build: Restrict check for CRC32C intrinsic to aarch64 (W. J. van der Laan) Pull request description: `crc32c`'s hardware accelerated code doesn't handle ARM 32-bit at all, but only 64-bit. Make the check in `configure.ac` check for this architecture explicitly. This change does not affect non-ARM architectures. For the release binaries, the current `configure.ac` check happens to work: it enables it on aarch64 but disables it for armhf. However some combination of compiler version and settings can cause this check to succeed on armhf (as reported on IRC). So make the 64-bit platform requirement explicit. (details: while the check already explicitly checks the `__crc32d` intrinsic, which strictly doesn't exist on 32-bit ARM, this is not enough! gcc happens to helpfully emulate it: > These built-in intrinsics for the ARMv8-A CRC32 extension are available when the -march=armv8-a+crc switch is used "uint32_t __crc32d (uint32_t, uint64_t)" Form of expected instruction(s): Two crc32w r0, r0, r0 instructions for AArch32 ) ACKs for top commit: luke-jr: re-tACK f2747d1 jarolrod: ACK f2747d1 Tree-SHA512: e5f05f05eeec310ac42685621d86862735586be66dc378db404ec9414ac5aaea7c53d76d76d875b15b11924eee6714076120c07b077183fd7af898704fd81823

`crc32c`'s hardware accelerated code doesn't handle ARM 32-bit at all. Make the check in `configure.ac` check for this architecture explicitly. For the release binaries, the current `configure.ac` check happens to work: it enables it on aarch64 but disables it for armhf. However some combination of compiler version and settings might ostensibly cause this check to succeed on armhf (as reported on IRC). So make the 64-bit platform requirement explicit. Github-Pull: bitcoin#23045 Rebased-From: f2747d1

fanquake · 2021-10-14T02:54:52Z

Being backported to 22.x in #23276.

`crc32c`'s hardware accelerated code doesn't handle ARM 32-bit at all. Make the check in `configure.ac` check for this architecture explicitly. For the release binaries, the current `configure.ac` check happens to work: it enables it on aarch64 but disables it for armhf. However some combination of compiler version and settings might ostensibly cause this check to succeed on armhf (as reported on IRC). So make the 64-bit platform requirement explicit. Github-Pull: bitcoin#23045 Rebased-From: f2747d1

269553f test: Call ceildiv helper with integer (Martin Zumsande) 2f60fc6 ci: Replace soon EOL hirsute with jammy (MarcoFalke) 801b0f0 build: patch qt to explicitly define previously implicit header include (Kittywhiskers Van Gogh) c768bfa tests: Calculate fees more similarly to CFeeRate::GetFee (Andrew Chow) f66bc42 tests: Test for assertion when feerate is rounded down (Andrew Chow) bd7e08e fees: Always round up fee calculated from a feerate (Andrew Chow) 227ae65 wallet: fix segfault by avoiding invalid default-ctored `external_spk_managers` entry (Sebastian Falbesoner) 282863a refactor: include a missing <limits> header in fs.cpp (Joan Karadimov) 7febe4f consensus: don't call GetBlockPos in ReadBlockFromDisk without lock (Jon Atack) c671c6f the result of CWallet::IsHDEnabled() was initialized with true. (Saibato) a5a1538 build, qt: Fix typo in QtInputSupport check (Hennadii Stepanov) c95b188 system: skip trying to set the locale on NetBSD (fanquake) c1cdedd guix: Fix powerpc64(le) dynamic linker name (Carl Dong) 92d44ff doc: Add 23061 release notes (MarcoFalke) db76db7 Fix (inverse) meaning of -persistmempool (MarcoFalke) 85c78e0 build: Restrict check for CRC32C intrinsic to aarch64 (W. J. van der Laan) Pull request description: Collecting backports for the 22.1 release. Currently: * #23045 * #23061 * #23148 * #22390 * #22820 * #22781 * #22895 * #23335 * #23333 * #22949 * #23580 * #23504 * #24239 ACKs for top commit: achow101: ACK 269553f Tree-SHA512: b3a57ea241be7a83488eeb032276f4cf82a0987aada906a82f94a20c4acf9f2397708249dcecbe1c7575e70d09c60b835233d4718af4013c7bc58896c618274c

…rch64 f2747d1 build: Restrict check for CRC32C intrinsic to aarch64 (W. J. van der Laan) Pull request description: `crc32c`'s hardware accelerated code doesn't handle ARM 32-bit at all, but only 64-bit. Make the check in `configure.ac` check for this architecture explicitly. This change does not affect non-ARM architectures. For the release binaries, the current `configure.ac` check happens to work: it enables it on aarch64 but disables it for armhf. However some combination of compiler version and settings can cause this check to succeed on armhf (as reported on IRC). So make the 64-bit platform requirement explicit. (details: while the check already explicitly checks the `__crc32d` intrinsic, which strictly doesn't exist on 32-bit ARM, this is not enough! gcc happens to helpfully emulate it: > These built-in intrinsics for the ARMv8-A CRC32 extension are available when the -march=armv8-a+crc switch is used "uint32_t __crc32d (uint32_t, uint64_t)" Form of expected instruction(s): Two crc32w r0, r0, r0 instructions for AArch32 ) ACKs for top commit: luke-jr: re-tACK f2747d1 jarolrod: ACK f2747d1 Tree-SHA512: e5f05f05eeec310ac42685621d86862735586be66dc378db404ec9414ac5aaea7c53d76d76d875b15b11924eee6714076120c07b077183fd7af898704fd81823

`crc32c`'s hardware accelerated code doesn't handle ARM 32-bit at all. Make the check in `configure.ac` check for this architecture explicitly. For the release binaries, the current `configure.ac` check happens to work: it enables it on aarch64 but disables it for armhf. However some combination of compiler version and settings might ostensibly cause this check to succeed on armhf (as reported on IRC). So make the 64-bit platform requirement explicit. Github-Pull: bitcoin#23045 Rebased-From: f2747d1

fanquake · 2022-06-09T11:57:18Z

Backported to 0.21 in #25318.

efb9f00 build: Restrict check for CRC32C intrinsic to aarch64 (W. J. van der Laan) cfb08c3 refactor: include a missing <limits> header in fs.cpp (Joan Karadimov) Pull request description: There might not be another 0.21.x release, however these are both straight forward changes. If this isn't merged, then the pulls can remain untagged for needing backport. Backports: - #23045 - #23335 ACKs for top commit: laanwj: ACK efb9f00 LarryRuane: utACK efb9f00 Tree-SHA512: 09be8f8ce90f862e2d408c5707a8387ca828fdd05a9814cfed5236030a3b33012e7d7a557c2ee3989db26922ad45cb8a307bdeba7ac8e34b5f21f0d46eda1955

hebasto · 2022-06-22T11:11:09Z

Should it be submitted to the upstream:

check_cxx_source_compiles("
#include <arm_acle.h>
#include <arm_neon.h>

int main() {
  __crc32cb(0, 0); __crc32ch(0, 0); __crc32cw(0, 0); __crc32cd(0, 0);
  vmull_p64(0, 0);
  return 0;
}
" HAVE_ARM64_CRC32C)

?

laanwj · 2022-06-22T11:21:11Z

Maybe! If they have the same check, and haven't actually fixed ARM32 support, they'll have the same problem.

hebasto · 2023-06-05T17:57:44Z

Should it be submitted to the upstream?

Maybe! If they have the same check, and haven't actually fixed ARM32 support, they'll have the same problem.

Done in google/crc32c#61.

…rch64 f2747d1 build: Restrict check for CRC32C intrinsic to aarch64 (W. J. van der Laan) Pull request description: `crc32c`'s hardware accelerated code doesn't handle ARM 32-bit at all, but only 64-bit. Make the check in `configure.ac` check for this architecture explicitly. This change does not affect non-ARM architectures. For the release binaries, the current `configure.ac` check happens to work: it enables it on aarch64 but disables it for armhf. However some combination of compiler version and settings can cause this check to succeed on armhf (as reported on IRC). So make the 64-bit platform requirement explicit. (details: while the check already explicitly checks the `__crc32d` intrinsic, which strictly doesn't exist on 32-bit ARM, this is not enough! gcc happens to helpfully emulate it: > These built-in intrinsics for the ARMv8-A CRC32 extension are available when the -march=armv8-a+crc switch is used "uint32_t __crc32d (uint32_t, uint64_t)" Form of expected instruction(s): Two crc32w r0, r0, r0 instructions for AArch32 ) ACKs for top commit: luke-jr: re-tACK f2747d1 jarolrod: ACK f2747d1 Tree-SHA512: e5f05f05eeec310ac42685621d86862735586be66dc378db404ec9414ac5aaea7c53d76d76d875b15b11924eee6714076120c07b077183fd7af898704fd81823

laanwj added Build system Needs backport (22.x) labels Sep 20, 2021

fanquake added the DrahtBot Guix build requested label Sep 20, 2021

laanwj force-pushed the 2021-09-arm64-crc32 branch from c1db96d to f6a7ed5 Compare September 20, 2021 10:36

laanwj added the Needs backport (0.21) label Sep 20, 2021

luke-jr reviewed Sep 20, 2021

View reviewed changes

laanwj force-pushed the 2021-09-arm64-crc32 branch from f6a7ed5 to f2747d1 Compare September 21, 2021 10:36

DrahtBot removed the DrahtBot Guix build requested label Sep 22, 2021

jarolrod reviewed Sep 22, 2021

View reviewed changes

laanwj merged commit cdce149 into bitcoin:master Sep 23, 2021

fanquake mentioned this pull request Oct 14, 2021

[22.x] Backports for 22.x #23276

Merged

fanquake removed the Needs backport (22.x) label Oct 14, 2021

reddink mentioned this pull request May 26, 2022

[4.22.x] Backports for 4.22.x reddcoin-project/reddcoin#20

Merged

fanquake mentioned this pull request Jun 9, 2022

0.21 Backports #25318

Merged

fanquake removed the Needs backport (0.21) label Jun 9, 2022

barton2526 mentioned this pull request Jul 19, 2022

build: Restrict check for CRC32C intrinsic to aarch64 gridcoin-community/Gridcoin-Research#2544

Merged

delta1 added a commit to delta1/elements that referenced this pull request Apr 23, 2023

Merge cdce149 into merged_master (Bitcoin PR bitcoin/bitcoin#23045)

6313268

ghost mentioned this pull request Apr 10, 2024

DigiByte Core ( BTC22.1 Backports for BTC Core 22 ) DigiByte-Core/digibyte#232

Merged

bitcoin locked and limited conversation to collaborators Jun 4, 2024

build: Restrict check for CRC32C intrinsic to aarch64 #23045

build: Restrict check for CRC32C intrinsic to aarch64 #23045

Uh oh!

Conversation

laanwj commented Sep 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Sep 20, 2021

Uh oh!

luke-jr commented Sep 20, 2021

Uh oh!

luke-jr Sep 20, 2021

Choose a reason for hiding this comment

Uh oh!

laanwj Sep 20, 2021

Choose a reason for hiding this comment

Uh oh!

laanwj commented Sep 20, 2021

Uh oh!

luke-jr commented Sep 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Sep 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Sep 22, 2021

Guix builds

Uh oh!

luke-jr commented Sep 22, 2021

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake commented Oct 14, 2021

Uh oh!

fanquake commented Jun 9, 2022

Uh oh!

hebasto commented Jun 22, 2022

Uh oh!

laanwj commented Jun 22, 2022

Uh oh!

hebasto commented Jun 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

laanwj commented Sep 20, 2021 •

edited

Loading

luke-jr commented Sep 20, 2021 •

edited

Loading

laanwj commented Sep 21, 2021 •

edited

Loading