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

Only run checks for intrinsics if optimizations are enabled. #906

Merged
merged 2 commits into from
Jun 13, 2021

Conversation

nmoinvaz
Copy link
Member

@nmoinvaz nmoinvaz commented Mar 20, 2021

Checks for instrinics supports and intrinsics flags are still being done if -DWITH_OPTIM=OFF. I have also cleaned it up and moved the intrinsics checks to detect-instrinsics.cmake.

@nmoinvaz nmoinvaz marked this pull request as draft March 20, 2021 21:12
@nmoinvaz nmoinvaz force-pushed the improvements/detect-intrinsics branch from f740522 to 9132e49 Compare March 20, 2021 21:14
@codecov
Copy link

codecov bot commented Mar 20, 2021

Codecov Report

Merging #906 (60d48f2) into develop (eb41bf4) will increase coverage by 0.18%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #906      +/-   ##
===========================================
+ Coverage    76.19%   76.37%   +0.18%     
===========================================
  Files           77       77              
  Lines         8254     8258       +4     
  Branches      1351     1351              
===========================================
+ Hits          6289     6307      +18     
+ Misses        1435     1419      -16     
- Partials       530      532       +2     
Flag Coverage Δ
macos_clang 68.29% <ø> (+0.09%) ⬆️
macos_gcc 67.17% <ø> (+0.08%) ⬆️
ubuntu_clang 69.30% <ø> (+0.09%) ⬆️
ubuntu_clang_debug 68.72% <ø> (+0.12%) ⬆️
ubuntu_clang_inflate_allow_invalid_dist 69.05% <ø> (+0.09%) ⬆️
ubuntu_clang_inflate_strict 69.30% <ø> (+0.09%) ⬆️
ubuntu_clang_mmap 69.30% <ø> (+0.09%) ⬆️
ubuntu_clang_msan 69.30% <ø> (+0.09%) ⬆️
ubuntu_clang_pigz 35.31% <ø> (-0.08%) ⬇️
ubuntu_clang_pigz_no_optim 37.74% <ø> (-0.09%) ⬇️
ubuntu_clang_pigz_no_threads 34.95% <ø> (-0.08%) ⬇️
ubuntu_gcc 68.48% <ø> (+0.21%) ⬆️
ubuntu_gcc_aarch64 68.73% <ø> (+0.23%) ⬆️
ubuntu_gcc_aarch64_compat_no_opt 66.92% <ø> (+0.20%) ⬆️
ubuntu_gcc_aarch64_no_acle 67.64% <ø> (+0.23%) ⬆️
ubuntu_gcc_aarch64_no_neon 67.94% <ø> (+0.19%) ⬆️
ubuntu_gcc_armhf 68.70% <ø> (+0.23%) ⬆️
ubuntu_gcc_armhf_compat_no_opt 66.89% <ø> (+0.09%) ⬆️
ubuntu_gcc_armhf_no_acle 68.86% <ø> (+0.23%) ⬆️
ubuntu_gcc_armhf_no_neon 69.11% <ø> (+0.19%) ⬆️
ubuntu_gcc_armsf 68.71% <ø> (+0.23%) ⬆️
ubuntu_gcc_armsf_compat_no_opt 66.89% <ø> (+0.09%) ⬆️
ubuntu_gcc_compat_no_opt 68.37% <ø> (+0.20%) ⬆️
ubuntu_gcc_mingw_i686 0.00% <ø> (ø)
ubuntu_gcc_mingw_x86_64 0.00% <ø> (ø)
ubuntu_gcc_no_avx2 68.72% <ø> (+0.18%) ⬆️
ubuntu_gcc_no_pclmulqdq 66.67% <ø> (+0.22%) ⬆️
ubuntu_gcc_no_sse2 67.89% <ø> (+0.21%) ⬆️
ubuntu_gcc_no_sse4 66.87% <ø> (+0.22%) ⬆️
ubuntu_gcc_o3 68.26% <ø> (+0.21%) ⬆️
ubuntu_gcc_osb 70.87% <ø> (+0.21%) ⬆️
ubuntu_gcc_pigz 35.15% <ø> (-0.03%) ⬇️
ubuntu_gcc_ppc 68.86% <ø> (+0.09%) ⬆️
ubuntu_gcc_ppc64 69.73% <ø> (+0.09%) ⬆️
ubuntu_gcc_ppc64le 68.42% <ø> (+0.19%) ⬆️
ubuntu_gcc_s390x 67.75% <ø> (+0.08%) ⬆️
ubuntu_gcc_sparc64 70.02% <ø> (+0.09%) ⬆️
win64_gcc 70.09% <ø> (+0.13%) ⬆️
win64_gcc_compat_no_opt 73.50% <ø> (+0.19%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
trees.c 74.59% <0.00%> (-0.28%) ⬇️
arch/arm/chunkset_neon.c 100.00% <0.00%> (ø)
inffast.c 88.69% <0.00%> (+0.41%) ⬆️
functable.c 81.25% <0.00%> (+10.93%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb41bf4...60d48f2. Read the comment docs.

@nmoinvaz nmoinvaz force-pushed the improvements/detect-intrinsics branch 3 times, most recently from b7a8eca to f24ec7a Compare March 21, 2021 01:00
@nmoinvaz nmoinvaz marked this pull request as ready for review March 21, 2021 13:25
@Dead2 Dead2 added the Next Devel Targeting next devel release (does not mean accepted) label Mar 22, 2021
@Dead2
Copy link
Member

Dead2 commented Mar 22, 2021

I like the cmake changes a lot.

The configure changes I am a little unsure about, I feel that they make the file less readable.
Perhaps do the similar thing and make functions?
Should make sure that they are the portable kind though: with or without function prefix

@Dead2 Dead2 added the cleanup Improving maintainability or removing code. label Mar 22, 2021
@nmoinvaz nmoinvaz force-pushed the improvements/detect-intrinsics branch 2 times, most recently from 94b0fcd to c482321 Compare March 23, 2021 01:31
@nmoinvaz
Copy link
Member Author

@Dead2 done. The check_*_intrinsics functions are sorted alphabetically like in CMake. Also have added a commit that will print out which static/shared object files are used (like CMake does) for verification.

@Dead2
Copy link
Member

Dead2 commented Mar 23, 2021

@nmoinvaz Wow, nice! Will do a full review at a later point, but looks good.

@@ -1466,6 +1470,8 @@ esac

echo "ARCH: ${ARCH}"
echo "Using arch directory: ${ARCHDIR}"
echo "Architecture-specific static object files:${ARCH_STATIC_OBJS}"
echo "Architecture-specific shared object files:${ARCH_SHARED_OBJS}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

For readability, there should be space after :.

Copy link
Member Author

Choose a reason for hiding this comment

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

Those variables already have a space at the beginning based on the way they are generated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

At least on modern x86, the lines are really long, over 200 characters. Even with wide-screen aspect ratio, that is very long.

configure Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@mtl1979
Copy link
Collaborator

mtl1979 commented May 8, 2021

Needs rebase.

@nmoinvaz nmoinvaz force-pushed the improvements/detect-intrinsics branch 2 times, most recently from caad935 to 5bff8ac Compare June 12, 2021 03:57
@nmoinvaz
Copy link
Member Author

Rebased.

configure Show resolved Hide resolved
@Dead2
Copy link
Member

Dead2 commented Jun 12, 2021

Needs rebase

@Dead2 Dead2 added the Rebase needed Please do a 'git rebase develop yourbranch' label Jun 12, 2021
@nmoinvaz nmoinvaz force-pushed the improvements/detect-intrinsics branch from 5bff8ac to f3313a0 Compare June 12, 2021 21:53
@nmoinvaz
Copy link
Member Author

nmoinvaz commented Jun 12, 2021

Rebased and added those () to configure script.

@nmoinvaz nmoinvaz removed the Rebase needed Please do a 'git rebase develop yourbranch' label Jun 12, 2021
@Dead2
Copy link
Member

Dead2 commented Jun 13, 2021

CI fails with:

../configure: line 1337: syntax error near unexpected token `fi'
../configure: line 1337: `        fi'

Seems I was wrong in suggesting (), as I find no references to documentation using those. I have always used them myself, but I guess that is actually not portable or even fully functional after all. Learned something there. Sorry about that.

@nmoinvaz nmoinvaz force-pushed the improvements/detect-intrinsics branch from f3313a0 to 60d48f2 Compare June 13, 2021 15:47
@nmoinvaz
Copy link
Member Author

Ok I have switched it back.

@Dead2 Dead2 merged commit e56238c into zlib-ng:develop Jun 13, 2021
@nmoinvaz
Copy link
Member Author

Nice!!

nmoinvaz added a commit to nmoinvaz/zlib-ng that referenced this pull request Jun 13, 2021
Dead2 pushed a commit that referenced this pull request Jun 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Improving maintainability or removing code. Next Devel Targeting next devel release (does not mean accepted)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants