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

Leqiao/add arm64 support #10981

Merged
merged 7 commits into from
Mar 25, 2022
Merged

Leqiao/add arm64 support #10981

merged 7 commits into from
Mar 25, 2022

Conversation

leqiao-1
Copy link
Contributor

@leqiao-1 leqiao-1 commented Mar 23, 2022

Description: add arm64 support

Resolve #10874
Motivation and Context

  • add new build artifacts

@snnn snnn self-requested a review March 23, 2022 16:04
snnn
snnn previously approved these changes Mar 23, 2022
Copy link
Member

@snnn snnn left a comment

Choose a reason for hiding this comment

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

LGTM.

@snnn
Copy link
Member

snnn commented Mar 23, 2022

@Craigacp , so the new jars will contains 5 folders like:
MicrosoftTeams-image

Does it look right?

@Craigacp
Copy link
Contributor

I think the JVM reports the platform as aarch64 for both Linux and macOS ARM64, so the osx-arm64 folder should be osx-aarch64. You can see our platform detection logic here - https://github.com/microsoft/onnxruntime/blob/master/java/src/main/java/ai/onnxruntime/OnnxRuntime.java#L88. Looks like this chunk (https://github.com/microsoft/onnxruntime/blame/master/cmake/onnxruntime_java.cmake#L81) has been emitting arm64 for a while, but the loader doesn't know what that is. I'm not sure how I missed that, though the library loading during the tests is a little different to the one from the jar.

I'm not sure what the cmake changes are doing exactly, if those are required for cross compiling then I guess we should modify the loader logic to emit arm64 rather than aarch64 when it's detected the OS as osx.

@snnn snnn self-requested a review March 23, 2022 22:10
@snnn snnn dismissed their stale review March 23, 2022 22:10

Need more work

@snnn
Copy link
Member

snnn commented Mar 23, 2022

OSX_ARCHITECTURES is a cmake variable at target level. It is an array of strings with length of one or two. It set target specific architectures for macOS. The valid values are:

  1. "arm64"
  2. "x86_64"
  3. "arm64;x86_64", which means arm64 and x86_64. In this case it will generate a binary for both archs. We call such binaries "universal2" binaries.

If you have set CMAKE_OSX_ARCHITECTURES, it will become the default value of OSX_ARCHITECTURES for every cmake target in your project.

(The above are not onnxruntime specific. It applies to all cmake projects)

So the lines 81-98 in onnxruntime_java.cmake try to extract arch information from OSX_ARCHITECTURES, then use it to set the JNI_ARCH variable which will be used later in line 136. JNI_ARCH variable determines JAVA_PACKAGE_DIR.

@Craigacp
Copy link
Contributor

Craigacp commented Mar 24, 2022

Ok. I think that there needs to be a chunk in onnxruntime_java.cmake like the block at line 95 which sets JNI_ARCH to aarch64. At that point it should emit the binary in the right place. I'm talking to some people in OpenJDK to figure out how JNI interacts with universal binaries, but I've not found an answer yet. When I do we might be able to add an extra osx-universal target to the loader and have it prefer that for macOS.

@snnn
Copy link
Member

snnn commented Mar 24, 2022

I think that there needs to be a chunk in onnxruntime_java.cmake like the block at line 95 which sets JNI_ARCH to aarch64. At that point it should emit the binary in the right place

@leqiao-1 please help make the change.

@@ -94,6 +94,8 @@ if(APPLE)
endif()
if(JNI_ARCH STREQUAL "x86_64")
set(JNI_ARCH x64)
elseif(JNI_ARCH STREQUAL "arm64")
Copy link
Member

Choose a reason for hiding this comment

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

What does the new package look like?
I suppose you will have "linux-aarch64" and "osx-aarch64"

Copy link
Member

@snnn snnn Mar 24, 2022

Choose a reason for hiding this comment

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

BTW, for your information in nuget packages both of them should be "arm64" instead of "aarch64".

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.

Problems with predictions on MacBook Air with M1 chip in Java project based on Maven
3 participants