build: Fix regression in “ARMv8 CRC32 intrinsics” test #28919

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:231120-crc-arm64 changing 1 files +1 −1
  1. hebasto commented at 1:49 pm on November 20, 2023: member

    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:

    • the master branch @ d752349029ec7a76f1fd440db2ec2e458d0f3c99:
    0checking whether C++ compiler accepts -march=armv8-a+crc... yes
    1checking whether C++ compiler accepts -march=armv8-a+crypto... yes
    2checking for ARMv8 CRC32 intrinsics... no
    3checking for ARMv8 SHA-NI intrinsics... yes
    
    • this PR:
    0checking whether C++ compiler accepts -march=armv8-a+crc+crypto... yes
    1checking whether C++ compiler accepts -march=armv8-a+crypto... yes
    2checking for ARMv8 CRC32 intrinsics... yes
    3checking for ARMv8 SHA-NI intrinsics... yes
    

    Guix build:

    0x86_64
    12afd81f540c6d3b36ff305e88bafe935e4272cd3efef3130aa69d49a0522541b  guix-build-228d6a2969e4/output/aarch64-linux-gnu/SHA256SUMS.part
    26c704d6d30d495adb3fb86befdb500eb389a02c1167163f14ab5c3c3e630e6b3  guix-build-228d6a2969e4/output/aarch64-linux-gnu/bitcoin-228d6a2969e4-aarch64-linux-gnu-debug.tar.gz
    3e4419963c9c0d99adc4e38538900b648f2c14f793b60c8ee2e6f5acc9d3fadd3  guix-build-228d6a2969e4/output/aarch64-linux-gnu/bitcoin-228d6a2969e4-aarch64-linux-gnu.tar.gz
    47d11052b6bd28cdf26d5f2a4987f02d32c93a061907bcd048fb6d161a0466ca9  guix-build-228d6a2969e4/output/dist-archive/bitcoin-228d6a2969e4.tar.gz
    
  2. build: Fix regression in "ARMv8 CRC32 intrinsics" test
    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.
    228d6a2969
  3. hebasto added the label Build system on Nov 20, 2023
  4. DrahtBot commented at 1:49 pm on November 20, 2023: contributor

    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.

  5. TheCharlatan commented at 11:31 am on November 21, 2023: contributor

    ACK 228d6a2969e4fcee573c9df7aad31550eab9c8d4

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

    Guix build (x86_64 and aarch64):

    02afd81f540c6d3b36ff305e88bafe935e4272cd3efef3130aa69d49a0522541b  guix-build-228d6a2969e4/output/aarch64-linux-gnu/SHA256SUMS.part
    16c704d6d30d495adb3fb86befdb500eb389a02c1167163f14ab5c3c3e630e6b3  guix-build-228d6a2969e4/output/aarch64-linux-gnu/bitcoin-228d6a2969e4-aarch64-linux-gnu-debug.tar.gz
    2e4419963c9c0d99adc4e38538900b648f2c14f793b60c8ee2e6f5acc9d3fadd3  guix-build-228d6a2969e4/output/aarch64-linux-gnu/bitcoin-228d6a2969e4-aarch64-linux-gnu.tar.gz
    
  6. TheCharlatan approved
  7. fanquake commented at 5:08 pm on November 22, 2023: 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):

    0checking for AVX2 intrinsics... no
    1checking for x86 SHA-NI intrinsics... no
    2checking whether C++ compiler accepts -march=armv8-a+crc... yes
    3checking whether C++ compiler accepts -march=armv8-a+crypto... yes
    4checking for ARMv8 CRC32 intrinsics... yes
    5checking for ARMv8 SHA-NI intrinsics... yes
    
  8. hebasto commented at 5:11 pm on November 22, 2023: 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)

    Right. Mind suggesting a more precise wording?

  9. fanquake commented at 5:16 pm on November 22, 2023: 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.

  10. fanquake merged this on Nov 22, 2023
  11. fanquake closed this on Nov 22, 2023

  12. hebasto commented at 5:22 pm on November 22, 2023: member
    Is it worth backporting to 26.x?
  13. fanquake referenced this in commit 6045f38dc8 on Nov 22, 2023
  14. fanquake commented at 5:22 pm on November 22, 2023: member
    Backported in #28872.
  15. fanquake referenced this in commit 180565266a on Nov 22, 2023
  16. hebasto deleted the branch on Nov 22, 2023
  17. fanquake referenced this in commit e4fef4ae65 on Nov 22, 2023
  18. hebasto referenced this in commit 02392e7062 on Nov 27, 2023
  19. hebasto referenced this in commit 399642ae2b on Dec 4, 2023

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-21 18:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me