Skip to content

build: Fix regression in "ARMv8 CRC32 intrinsics" test #28919

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 22, 2023
Merged

build: Fix regression in "ARMv8 CRC32 intrinsics" test #28919

merged 1 commit into from
Nov 22, 2023

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Nov 20, 2023

In the master branch, the aarch64 binaries lack support for CRC32 intrinsics.

The vmull_p64 is a part of the Crypto extensions from the ACLE. They are optional extensions, so they get enabled with a +crypto for architecture flags.

The regression was introduced in #26183 (v25.0).

The ./configure script log excerpts:

checking whether C++ compiler accepts -march=armv8-a+crc... yes
checking whether C++ compiler accepts -march=armv8-a+crypto... yes
checking for ARMv8 CRC32 intrinsics... no
checking for ARMv8 SHA-NI intrinsics... yes
  • this PR:
checking whether C++ compiler accepts -march=armv8-a+crc+crypto... yes
checking whether C++ compiler accepts -march=armv8-a+crypto... yes
checking for ARMv8 CRC32 intrinsics... yes
checking for ARMv8 SHA-NI intrinsics... yes

Guix build:

x86_64
2afd81f540c6d3b36ff305e88bafe935e4272cd3efef3130aa69d49a0522541b  guix-build-228d6a2969e4/output/aarch64-linux-gnu/SHA256SUMS.part
6c704d6d30d495adb3fb86befdb500eb389a02c1167163f14ab5c3c3e630e6b3  guix-build-228d6a2969e4/output/aarch64-linux-gnu/bitcoin-228d6a2969e4-aarch64-linux-gnu-debug.tar.gz
e4419963c9c0d99adc4e38538900b648f2c14f793b60c8ee2e6f5acc9d3fadd3  guix-build-228d6a2969e4/output/aarch64-linux-gnu/bitcoin-228d6a2969e4-aarch64-linux-gnu.tar.gz
7d11052b6bd28cdf26d5f2a4987f02d32c93a061907bcd048fb6d161a0466ca9  guix-build-228d6a2969e4/output/dist-archive/bitcoin-228d6a2969e4.tar.gz

@hebasto
build: Fix regression in "ARMv8 CRC32 intrinsics" test
228d6a2 
The `vmull_p64` is a part of the Crypto extensions from the ACLE. They
are optional extensions, so they get enabled with a `+crypto` for
architecture flags.
@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 20, 2023 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK TheCharlatan

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@TheCharlatan
Copy link
Contributor

ACK 228d6a2

I checkd that the bianry now contains both crc and crypto intrinsic opcodes.

Guix build (x86_64 and aarch64):

2afd81f540c6d3b36ff305e88bafe935e4272cd3efef3130aa69d49a0522541b  guix-build-228d6a2969e4/output/aarch64-linux-gnu/SHA256SUMS.part
6c704d6d30d495adb3fb86befdb500eb389a02c1167163f14ab5c3c3e630e6b3  guix-build-228d6a2969e4/output/aarch64-linux-gnu/bitcoin-228d6a2969e4-aarch64-linux-gnu-debug.tar.gz
e4419963c9c0d99adc4e38538900b648f2c14f793b60c8ee2e6f5acc9d3fadd3  guix-build-228d6a2969e4/output/aarch64-linux-gnu/bitcoin-228d6a2969e4-aarch64-linux-gnu.tar.gz

@fanquake
Copy link
Member

The vmull_p64 is a part of the Crypto extensions from the ACLE. They are optional extensions, so they get enabled with a +crypto for architecture flags.

I guess this isn't true for all aarch64, as the checks currently work fine, for example, on Apple arm64 (M1):

checking for AVX2 intrinsics... no
checking for x86 SHA-NI intrinsics... no
checking whether C++ compiler accepts -march=armv8-a+crc... yes
checking whether C++ compiler accepts -march=armv8-a+crypto... yes
checking for ARMv8 CRC32 intrinsics... yes
checking for ARMv8 SHA-NI intrinsics... yes

@hebasto
Copy link
Member Author

hebasto commented Nov 22, 2023

The vmull_p64 is a part of the Crypto extensions from the ACLE. They are optional extensions, so they get enabled with a +crypto for architecture flags.

I guess this isn't true for all aarch64, as the checks currently work fine, for example, on Apple arm64 (M1)

Right. Mind suggesting a more precise wording?

@fanquake
Copy link
Member

I guess this isn't true for all aarch64, as the checks currently work fine, for example, on Apple arm64 (M1):

Actually, I think this is just an Apple special case, and because Apple Clang just enables +crypto and +crc as part of the default compile flags.

@fanquake fanquake merged commit ddc4b98 into bitcoin:master Nov 22, 2023
@hebasto
Copy link
Member Author

hebasto commented Nov 22, 2023

Is it worth backporting to 26.x?

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Nov 22, 2023
@hebasto @fanquake
build: Fix regression in "ARMv8 CRC32 intrinsics" test
6045f38 
The `vmull_p64` is a part of the Crypto extensions from the ACLE. They
are optional extensions, so they get enabled with a `+crypto` for
architecture flags.

Github-Pull: bitcoin#28919
Rebased-From: 228d6a2
@fanquake
Copy link
Member

Backported in #28872.

This was referenced Nov 22, 2023
Merged
Merged
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Nov 22, 2023
@hebasto @fanquake
build: Fix regression in "ARMv8 CRC32 intrinsics" test
1805652 
The `vmull_p64` is a part of the Crypto extensions from the ACLE. They
are optional extensions, so they get enabled with a `+crypto` for
architecture flags.

Github-Pull: bitcoin#28919
Rebased-From: 228d6a2
@hebasto hebasto deleted the 231120-crc-arm64 branch November 22, 2023 17:30
fanquake added a commit that referenced this pull request Nov 22, 2023
@fanquake
Merge #28872: [26.x] Changes for rc3
e4fef4a 
2f86d30 doc: update release notes for v26.0rc3 (fanquake)
3b6c7f2 doc: update manual pages for v26.0rc3 (fanquake)
3db4d1c build: bump version to v26.0rc3 (fanquake)
6045f38 build: Fix regression in "ARMv8 CRC32 intrinsics" test (Hennadii Stepanov)
5eaa179 ci: Avoid toolset ambiguity that MSVC can't handle (Hennadii Stepanov)
55af112 p2p: do not make automatic outbound connections to addnode peers (Jon Atack)
5e0bcc1 ci: Switch from `apt` to `apt-get` (Hennadii Stepanov)
437a531 ci: Update apt cache (Hennadii Stepanov)
1488648 pool: change memusage_test to use int64_t, add allocation check (Martin Leitner-Ankerl)
bcc183c pool: make sure PoolAllocator uses the correct alignment (Martin Leitner-Ankerl)
7dda499 doc: regenerate example bitcoin.conf (fanquake)
5845331 doc: rewrite explanation for -par= (fanquake)

Pull request description:

  Currently backports:
  * #28858
  * #28895 (partial)
  * #28913
  * #28905
  * #28919
  * #28925

  Also includes changes for rc3, and reintegrating the release-notes.

ACKs for top commit:
  hebasto:
    re-ACK 2f86d30, only #28919 backported since my [recent](#28872 (review)) review.
  TheCharlatan:
    ACK 2f86d30

Tree-SHA512: 43c91b344d37f582081ac184ac59cf76c741317b2b69a24fcd4287eefa8333e20c545e150798f4057d6f4ac8e70ed9cba1c8dd9777b11c1cf8992cce09108727
hebasto added a commit to hebasto/bitcoin that referenced this pull request Nov 27, 2023
@hebasto
fixup! cmake: Build crc32c static library
02392e7 
See: bitcoin#28919
@hebasto hebasto mentioned this pull request Nov 27, 2023
Merged
hebasto added a commit to hebasto/bitcoin that referenced this pull request Dec 4, 2023
@hebasto
Merge #57: cmake: Backport changes from master
399642a 
02392e7 fixup! cmake: Build `crc32c` static library (Hennadii Stepanov)
a682425 fixup! cmake: Check system symbols (Hennadii Stepanov)

Pull request description:

  The first commit fixes a regression, which makes tests running with the empty `CMAKE_REQUIRED_FLAGS` variable. It was introduced during updating the code for the minimum CMake 3.16.

  The second commit mirrors bitcoin#28919.

ACKs for top commit:
  pablomartin4btc:
    tACK 02392e7

Tree-SHA512: 80a8fb13367b3b5be7db2663dc82667400b527c49ea22b4c07abcc916797e0cdbc1348d28bbd02e7b7f4c34f5e7d816947517d3998449e5fa963ba84f8a0690d
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Dec 11, 2023
@hebasto @fanquake
build: Fix regression in "ARMv8 CRC32 intrinsics" test
b34f54d 
The `vmull_p64` is a part of the Crypto extensions from the ACLE. They
are optional extensions, so they get enabled with a `+crypto` for
architecture flags.

Github-Pull: bitcoin#28919
Rebased-From: 228d6a2
@bitcoin bitcoin locked and limited conversation to collaborators Dec 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@TheCharlatan TheCharlatan TheCharlatan 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.
4 participants
@hebasto @DrahtBot @TheCharlatan @fanquake