Reset compiler flags after completing intrinsic tests by Dead2 · Pull Request #2082 · zlib-ng/zlib-ng

Dead2 · 2026-01-09T14:20:22Z

Add missing reset of compiler flags after completing a test, avoids the next test inheriting the previous flags.

coderabbitai · 2026-01-09T14:23:41Z

Walkthrough

Reset CMAKE_REQUIRED_FLAGS after the AVX512 and SSSE3 intrinsics check macros in cmake/detect-intrinsics.cmake so subsequent checks run with a cleared flag state.

Changes

Cohort / File(s)	Summary
CMake Build Configuration `cmake/detect-intrinsics.cmake`	Added `set(CMAKE_REQUIRED_FLAGS)` calls to reset `CMAKE_REQUIRED_FLAGS` after the AVX512 and SSSE3 intrinsics checks, clearing per-check compiler flags for subsequent macro invocations.

Suggested labels

cleanup

Suggested reviewers

nmoinvaz

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: resetting compiler flags after intrinsic tests are completed, which matches the two additions of CMAKE_REQUIRED_FLAGS resets in the cmake file.
Description check	✅ Passed	The description is directly related to the changeset, explaining that missing compiler flag resets were added to prevent flag inheritance between tests.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch fix-avx512-cmake-detect

📜 Recent review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 523982c and 2c17bc8.

📒 Files selected for processing (1)

cmake/detect-intrinsics.cmake

🚧 Files skipped from review as they are similar to previous changes (1)

cmake/detect-intrinsics.cmake

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

cmake/detect-intrinsics.cmake (1)
604-627: Same issue exists in check_ssse3_intrinsics macro.

This macro sets CMAKE_REQUIRED_FLAGS at line 617 but is missing the reset before endmacro(). The SSSE3 flags will leak into subsequent checks (starting with check_sse41_intrinsics).
🔧 Proposed fix
         HAVE_SSSE3_INTRIN
     )
+    set(CMAKE_REQUIRED_FLAGS)
 endmacro()

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 160d786 and 5338c9a.

📒 Files selected for processing (1)

cmake/detect-intrinsics.cmake

🧰 Additional context used

🧠 Learnings (4)

📓 Common learnings

Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/x86_intrins.h:114-117
Timestamp: 2025-02-23T16:51:54.545Z
Learning: In x86/x86_intrins.h, the Clang macros for _mm_cvtsi64x_si128 and _mm_cvtsi128_si64x don't need additional MSVC guards since MSVC's implementation is already protected by `defined(_MSC_VER) && !defined(__clang__)`, making them mutually exclusive.

Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1805
File: inffast_tpl.h:257-262
Timestamp: 2024-10-29T02:22:52.846Z
Learning: In `inffast_tpl.h`, when AVX512 is enabled, the branch involving `chunkcopy_safe` is intentionally eliminated to optimize performance.

📚 Learning: 2025-02-23T16:51:54.545Z

Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/x86_intrins.h:114-117
Timestamp: 2025-02-23T16:51:54.545Z
Learning: In x86/x86_intrins.h, the Clang macros for _mm_cvtsi64x_si128 and _mm_cvtsi128_si64x don't need additional MSVC guards since MSVC's implementation is already protected by `defined(_MSC_VER) && !defined(__clang__)`, making them mutually exclusive.

Applied to files:

cmake/detect-intrinsics.cmake

📚 Learning: 2025-10-08T18:04:33.839Z

Learnt from: mtl1979
Repo: zlib-ng/zlib-ng PR: 1977
File: cmake/detect-intrinsics.cmake:142-180
Timestamp: 2025-10-08T18:04:33.839Z
Learning: MSVC Visual Studio version 18.0.0 (build 11018.127) and later supports the AVX2-VNNI intrinsic _mm256_dpbusd_epi32 via zmmintrin.h header and can compile it with /arch:AVX2 flag.

Applied to files:

cmake/detect-intrinsics.cmake

📚 Learning: 2024-10-29T02:22:52.846Z

Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1805
File: inffast_tpl.h:257-262
Timestamp: 2024-10-29T02:22:52.846Z
Learning: In `inffast_tpl.h`, when AVX512 is enabled, the branch involving `chunkcopy_safe` is intentionally eliminated to optimize performance.

Applied to files:

cmake/detect-intrinsics.cmake

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)

GitHub Check: macOS GCC UBSAN (ARM64)
GitHub Check: macOS GCC UBSAN (ARM64)
GitHub Check: macOS GCC UBSAN (ARM64)
GitHub Check: macOS GCC UBSAN (ARM64)
GitHub Check: macOS GCC UBSAN (ARM64)
GitHub Check: macOS GCC UBSAN (ARM64)
GitHub Check: macOS GCC UBSAN (ARM64)
GitHub Check: macOS GCC UBSAN (ARM64)
GitHub Check: macOS GCC UBSAN (ARM64)
GitHub Check: macOS GCC UBSAN (ARM64)

🔇 Additional comments (1)

cmake/detect-intrinsics.cmake (1)

174-174: Critical fix: Prevents AVX512 flags from leaking into subsequent checks.

This correctly resets CMAKE_REQUIRED_FLAGS after the AVX512 intrinsics check, matching the pattern used in all other intrinsics check macros. Without this reset, the AVX512 flags would persist and contaminate subsequent intrinsics detection, potentially causing false results or compilation errors.

codecov · 2026-01-09T14:56:39Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.69%. Comparing base (88e314c) to head (2c17bc8).
⚠️ Report is 2 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2082      +/-   ##
===========================================
+ Coverage    81.39%   82.69%   +1.30%     
===========================================
  Files          168      168              
  Lines        13323    13323              
  Branches      3322     3322              
===========================================
+ Hits         10844    11018     +174     
+ Misses        1487     1313     -174     
  Partials       992      992

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mtl1979

LGTM

avoids the next test inheriting the previous flags.

Dead2 added the Build Env label Jan 9, 2026

coderabbitai bot reviewed Jan 9, 2026

View reviewed changes

Dead2 force-pushed the fix-avx512-cmake-detect branch from 5338c9a to 523982c Compare January 9, 2026 15:13

Dead2 changed the title ~~Reset compiler flags after completing AVX512 test~~ Reset compiler flags after completing intrinsic tests Jan 9, 2026

Dead2 mentioned this pull request Jan 9, 2026

cmake: use bracket strings #1945

Draft

2 tasks

nmoinvaz approved these changes Jan 9, 2026

View reviewed changes

mtl1979 approved these changes Jan 9, 2026

View reviewed changes

Add missing resets of compiler flags after completing each test,

2c17bc8

avoids the next test inheriting the previous flags.

Dead2 force-pushed the fix-avx512-cmake-detect branch from 523982c to 2c17bc8 Compare January 10, 2026 00:40

Dead2 merged commit bbd2c90 into develop Jan 10, 2026
298 of 299 checks passed

Dead2 mentioned this pull request Jan 30, 2026

2.3.3 Release #2134

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Reset compiler flags after completing intrinsic tests#2082

Reset compiler flags after completing intrinsic tests#2082
Dead2 merged 1 commit intodevelopfrom
fix-avx512-cmake-detect

Dead2 commented Jan 9, 2026

Uh oh!

coderabbitai bot commented Jan 9, 2026 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

codecov bot commented Jan 9, 2026 •

edited

Loading

Uh oh!

mtl1979 left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

Dead2 commented Jan 9, 2026

Uh oh!

coderabbitai bot commented Jan 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Suggested labels

Suggested reviewers

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jan 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mtl1979 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coderabbitai bot commented Jan 9, 2026 •

edited

Loading

codecov bot commented Jan 9, 2026 •

edited

Loading