Skip to content

[CMake] Only enable LLVM_ENABLE_ONDISK_CAS_default for 64-bit or larger architectures #5883

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 27, 2023
Merged

[CMake] Only enable LLVM_ENABLE_ONDISK_CAS_default for 64-bit or larger architectures #5883

merged 1 commit into from
Jan 27, 2023

Conversation

finagolfin
Copy link
Member

This currently breaks when building the Swift toolchain for Android armv7, so I have it disabled there by default for the last couple months.

Instead, this pull should only enable it by default for commonly used 64-bit architectures, until someone gets it working for 32-bit architectures.

@cachemeifyoucan, please review whenever you get back from break.

@finagolfin finagolfin mentioned this pull request Dec 26, 2022
Merged
@finagolfin
Copy link
Member Author

Pinging @cachemeifyoucan, looks like you're back, please review.

@cachemeifyoucan
Copy link

Changing the default sounds good to me. Please make sure also making the same change to stable/20221013 and next branch too.

In terms of the actual change, it might be better just check CMAKE_SIZEOF_VOID_P rather than match architectures, which might not taking care of some other platforms that we are not aware of.

@finagolfin
Copy link
Member Author

Please make sure also making the same change to stable/20221013 and next branch too.

Alright, will submit those next, once this is in.

it might be better just check CMAKE_SIZEOF_VOID_P

Done.

@finagolfin finagolfin changed the title [CMake] Only set LLVM_ENABLE_ONDISK_CAS_default for tested 64-bit architectures [CMake] Only enable LLVM_ENABLE_ONDISK_CAS_default for 64-bit architectures Jan 13, 2023
cachemeifyoucan
cachemeifyoucan approved these changes Jan 13, 2023
View reviewed changes
Copy link

@cachemeifyoucan cachemeifyoucan left a comment

Choose a reason for hiding this comment

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

Nit for comparison. Otherwise, LGTM

llvm/CMakeLists.txt Outdated
@@ -625,7 +625,7 @@ option (LLVM_ENABLE_SPHINX "Use Sphinx to generate llvm documentation." OFF)
option (LLVM_ENABLE_OCAMLDOC "Build OCaml bindings documentation." ON)
option (LLVM_ENABLE_BINDINGS "Build bindings." ON)

if(UNIX)
if(UNIX AND CMAKE_SIZEOF_VOID_P EQUAL 8)
Copy link

Choose a reason for hiding this comment

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

Maybe GREATER_EQUAL?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@finagolfin finagolfin changed the title [CMake] Only enable LLVM_ENABLE_ONDISK_CAS_default for 64-bit architectures [CMake] Only enable LLVM_ENABLE_ONDISK_CAS_default for 64-bit or larger architectures Jan 13, 2023
@finagolfin
Copy link
Member Author

@cachemeifyoucan, please run the CI and merge, I will submit for the other branches you asked for.

@cachemeifyoucan
Copy link

@cachemeifyoucan, please run the CI and merge, I will submit for the other branches you asked for.

Sure. Please make this clear earlier next time~

@cachemeifyoucan
Copy link

@swift-ci please test

@cachemeifyoucan cachemeifyoucan merged commit f0cc25c into swiftlang:stable/20220421 Jan 27, 2023
@cachemeifyoucan
Copy link

Sorry I forgot about this. Merged.

@finagolfin finagolfin deleted the cas-32-bit branch February 1, 2023 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cachemeifyoucan cachemeifyoucan cachemeifyoucan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@finagolfin @cachemeifyoucan