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

add iOS and armv8 build #1014

Merged
merged 24 commits into from
Feb 16, 2024
Merged

add iOS and armv8 build #1014

merged 24 commits into from
Feb 16, 2024

Conversation

vrnmthr
Copy link
Contributor

@vrnmthr vrnmthr commented Nov 7, 2023

Description

Adds iOS compilation support to marian, and adds memory-mapping support to TranslateService in translator.h. Everything should be backwards-compatible. Compilation for iOS is intended to be done using the added cmake toolchain file. It is detected via CMAKE_SYSTEM_NAME in the cmake build.

Also fixes one bug in the memory mapping in binary.cpp that allows us to read mmap files with fewer memory allocations.

Added dependencies: simd_utils, ruy

How to test

Build cmake . -Bbuild -DCOMPILE_CUDA=off -DCMAKE_TOOLCHAIN_FILE=./cmake/ios.toolchain.cmake -DPLATFORM=OS64 -DDEPLOYMENT_TARGET=13.0 for iOS

Build cmake . -Bbuild -DCOMPILE_CUDA=off -DARM=on for ARM

Checklist

  • I have tested the code manually
  • I have run regression tests
  • I have read and followed CONTRIBUTING.md
  • I have updated CHANGELOG.md

@vrnmthr vrnmthr changed the title [draft] build for iOS add iOS build Dec 1, 2023
Copy link
Member

@snukky snukky left a comment

Choose a reason for hiding this comment

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

LGTM, I only had minor suggestions and clarifying questions.

.gitmodules Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
src/3rd_party/CMakeLists.txt Show resolved Hide resolved
src/3rd_party/spdlog/async_logger.h Outdated Show resolved Hide resolved
src/functional/operators.h Outdated Show resolved Hide resolved
src/translator/translator.h Show resolved Hide resolved
@vrnmthr vrnmthr changed the title add iOS build add iOS and armv8 build Feb 7, 2024
Copy link
Member

@emjotde emjotde left a comment

Choose a reason for hiding this comment

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

Request for small clarifications, will be good to merge afterwards.

src/3rd_party/sentencepiece Outdated Show resolved Hide resolved
src/3rd_party/faiss/VectorTransform.cpp Show resolved Hide resolved
@@ -109,7 +109,7 @@ void loadItems(const std::string& fileName, std::vector<io::Item>& items) {

io::Item getItem(const void* current, const std::string& varName) {
std::vector<io::Item> items;
loadItems(current, items);
loadItems(current, items, /*mapped=*/true);
Copy link
Member

Choose a reason for hiding this comment

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

How sure are we this is always OK?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have tested this with the iOS build and with through our quicksand paths and have had no issues. If there are other scenarios that you know of that you want to test to ensure they don't break we can try and test them as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in binary.cpp in the loadItems function there is a comment that indicates that perhaps there may be some hardware-specific compatibility we should test?
// @TOOD: verify this actually works for the hardware-specific ones like intgemm8avx2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you are uncomfortable with this, we can also pass the mapped variable through the callchain to let callers set it as they see fit. we need this change in iOS otherwise loading/parsing the YAML is too slow

Copy link
Member

Choose a reason for hiding this comment

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

There have been a lot of changes in the mmapping interface in internal master, so we will take a look in the merge PR what is going on there.

@emjotde emjotde merged commit 2d067af into master Feb 16, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants