Skip to content
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.

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

Provide an inline asm fallback for the ARMv8 intrinsics #1697

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

ccawley2011
Copy link
Contributor

The configure checks have also been modified to better match the ARMv6 code.

Copy link

codecov bot commented Feb 29, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 83.04%. Comparing base (93b870f) to head (cc24587).
Report is 9 commits behind head on develop.

Files Patch % Lines
test/test_crc32.cc 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #1697   +/-   ##
========================================
  Coverage    83.03%   83.04%           
========================================
  Files          134      134           
  Lines        10336    10336           
  Branches      2813     2813           
========================================
+ Hits          8583     8584    +1     
- Misses        1054     1057    +3     
+ Partials       699      695    -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mtl1979
Copy link
Collaborator

mtl1979 commented Mar 2, 2024

Renaming ACLE to just ARMV8 is misleading... ARMv8 is a lot more than just ACLE CRC instructions.

@nmoinvaz nmoinvaz added Build Env Architecture Architecture specific labels Mar 6, 2024
@nmoinvaz
Copy link
Member

nmoinvaz commented Mar 6, 2024

I'm not familar enough with ARM, why rename ACLE to ARMv8?

@ccawley2011
Copy link
Contributor Author

I'm not familar enough with ARM, why rename ACLE to ARMv8?

ACLE is the name of the header for non-NEON intrinsics rather than a specific CPU extension. It's used for both ARMv8 CRC32 instructions and ARMv6 SIMD instructions, however the build system for zlib-ng only uses ARM_ACLE to refer to the ARMv8 code. The ARMv6 code uses a separate option.

@mtl1979
Copy link
Collaborator

mtl1979 commented Mar 10, 2024

We can't just rename command-line options for configure or CMake build stage, as those have to be backwards compatible. Otherwise we break build for every project that use zlib-ng. Adding support for inline asm fallback should be separated as own pull request, as it is less controversial, even though we have no precedence for supporting mismatched toolchains, which is clearly the case here as GNU assembler would be newer than GNU C compiler (GCC).

@Dead2
Copy link
Member

Dead2 commented Apr 3, 2024

Renaming should be split into a separate PR from the added features/fixes.

@Un1q32
Copy link
Contributor

Un1q32 commented Apr 24, 2024

The improved acle/armv8 check fixes a false positive when building for 32 bit arm iOS, I ripped out the improved check and put it in https://github.com/Un1q32/zlib-ng/tree/acle, I would make a PR with just that but I'll wait to see if the author of this PR finishes the main PR

@Un1q32
Copy link
Contributor

Un1q32 commented May 17, 2024

No activity here for a while, gonna make a PR for the bit I ripped out

@Un1q32 Un1q32 mentioned this pull request May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants