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

Disable ISA-L on aarch64 architectures #49256

Merged
merged 1 commit into from
Apr 27, 2023

Conversation

jrdi
Copy link
Contributor

@jrdi jrdi commented Apr 27, 2023

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Enabling ISA-L by default except on aarch64 architectures to fix compilation

Closes #49254

@robot-ch-test-poll3 robot-ch-test-poll3 added pr-not-for-changelog This PR should not be mentioned in the changelog submodule changed At least one submodule changed in this PR. labels Apr 27, 2023
@jrdi jrdi marked this pull request as ready for review April 27, 2023 10:40
@alexey-milovidov alexey-milovidov added the can be tested Allows running workflows for external contributors label Apr 27, 2023
@Algunenano
Copy link
Member

It seems that the problem is that CH is adding the ASM for different instruction sets unconditionally:

${ISAL_SOURCE_DIR}/mem/mem_zero_detect_avx.asm
${ISAL_SOURCE_DIR}/mem/mem_zero_detect_avx2.asm
${ISAL_SOURCE_DIR}/mem/mem_zero_detect_avx512.asm
${ISAL_SOURCE_DIR}/mem/mem_zero_detect_sse.asm

The original build system does distinguish between architectures:

https://github.com/ClickHouse/isa-l/blob/9f2b68f05752097f0f16632fc4a9a86950831efd/mem/Makefile.am#L32-L41

So contrib/isa-l-cmake/CMakeLists.txt should do too. But let's also have the option to disable it (added to this PR) like pretty much any other library. Even more since this is adding extra build dependencies.

@alexey-milovidov alexey-milovidov self-assigned this Apr 27, 2023
@alexey-milovidov alexey-milovidov merged commit e2b28e2 into ClickHouse:master Apr 27, 2023
238 of 252 checks passed
@jrdi jrdi deleted the make-isal-optional branch April 27, 2023 13:39
rschu1ze added a commit that referenced this pull request May 1, 2023
Follow-up to #49256. More 'modern', i.e. uses a CMake TARGET exists
check instead of an internal variable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
can be tested Allows running workflows for external contributors pr-not-for-changelog This PR should not be mentioned in the changelog submodule changed At least one submodule changed in this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aarch64 build broken after #48833
4 participants