Revert "Change default x86 build target from x86-64-v2 (SSE4.2) to x86-64-v3 (AVX2)"#104971
Merged
Merged
Conversation
…6-64-v3 (AVX2)" This reverts PR ClickHouse#90043 (merge commit e21a84e) due to performance regressions observed in the MSan build. Note: a post-merge follow-up commit (9df07df, "Pass nm/ar/objcopy paths from CMake to localize_rust_c_symbols.sh") added an `NM_PATH` lookup in `cmake/tools.cmake` for the now-removed `cmake/localize_rust_c_symbols.sh` helper. The lookup is left in place by this revert; it is unused but harmless and can be cleaned up separately.
Contributor
|
Workflow [PR], commit [d66039c] Summary: ❌
AI ReviewSummaryThis PR reverts Findings❌ Blockers
Final Verdict❌ Changes requested |
clickhouse-gh
Bot
added
pr-backward-incompatible
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
The reverted Morton encode/decode files still referenced `DECLARE_AVX2_SPECIFIC_CODE`, a macro that was renamed to `DECLARE_X86_64_V3_SPECIFIC_CODE` in an earlier refactor and never restored. With the default `X86_ARCH_LEVEL=2` build the offending block is preprocessed away (`__BMI2__` is undefined), but configuring with `-DX86_ARCH_LEVEL=3` or higher would fail to compile. Switch both call sites to the existing `DECLARE_X86_64_V3_SPECIFIC_CODE` macro so the BMI2 multitarget block compiles correctly when enabled.
| __attribute__((always_inline)) | ||
| inline bool cpu_has_fma() | ||
| { | ||
| static const bool has_fma = DB::CPU::haveFMA(); |
Contributor
There was a problem hiding this comment.
cpu_has_fma is not a sufficient gate for the x86_64_v3 math implementation. The x86_64_v3 variant is built with AVX2-level requirements, but this check only tests FMA, so a host that has FMA but lacks some x86_64_v3 features can still be routed to __llvm_libc__x86_64_v3::* and hit illegal-instruction faults at runtime.
Please gate this on the full x86_64_v3 capability check (for example isArchSupported(TargetArch::x86_64_v3)), with x86_64_v2 as fallback.
Contributor
LLVM Coverage Report
Changed lines: 75.49% (465/616) | lost baseline coverage: 18 line(s) · Uncovered code |
Merged
via the queue into
ClickHouse:master
with commit May 15, 2026
741871e
165 of 167 checks passed
robot-ch-test-poll2
added
the
pr-synced-to-cloud
This was referenced May 15, 2026
Merged
Draft
1 task
groeneai
added a commit
to groeneai/ClickHouse
that referenced
this pull request
May 20, 2026
… WasmEdge MSan CI Algunenano's PR ClickHouse#104971 reverts the x86v3 default; that fixed the chronic DB::SystemLogQueue<DB::QueryLogElement>::waitFlush 180s timeout family on Stateless tests (amd_msan, WasmEdge, *) which was the only failing check on this PR's prior CI run (2026-05-15 02:15 UTC). Master WasmEdge MSan SystemLog flush timeouts dropped from 33/day (2026-05-13) to 0/day (2026-05-16+) after PR ClickHouse#104971 merged at 2026-05-15 08:57 UTC. Merging master to re-trigger CI on the post-revert build so PR ClickHouse#104968 can be evaluated cleanly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Revert PR #90043 due to performance regressions in the MSan build.
Reference: #104947 (comment)
Note: a post-merge follow-up commit (
9df07df5a0a, "Pass nm/ar/objcopy paths from CMake to localize_rust_c_symbols.sh") added anNM_PATHlookup incmake/tools.cmakefor the now-removedcmake/localize_rust_c_symbols.shhelper. The lookup is left in place by this revert; it is unused but harmless and can be cleaned up separately.Closes #104866
Closes #104873
Closes #104874
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Revert the default x86 build target back to x86-64-v2 (SSE4.2). The previous switch to x86-64-v3 (AVX2) is being rolled back.