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 blake3 NEON instructions on linux arm64 #19384

Closed
wants to merge 4 commits into from

Conversation

mauriciogg
Copy link
Contributor

otherwise getting libunix_jni.so: undefined symbol: blake3_hash_many_neon

see d0de5e0

otherwise getting libunix_jni.so: undefined symbol: blake3_hash_many_neon

see bazelbuild@d0de5e0
@github-actions github-actions bot added the awaiting-review PR is awaiting review from an assigned reviewer label Aug 31, 2023
Comment on lines 60 to 62
"@bazel_tools//src/conditions:linux_aarch64": [
"-DBLAKE3_USE_NEON=0",
],
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we instead include c/blake3_neon.c like we do in the @bazel_tools//src/conditions:darwin_arm64 condition above, and set -DBLAKE3_USE_NEON=1? Same for windows arm64 @meteorcloudy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that works, I can make the change for windows here as well, but I'll wait for @meteorcloudy comments.

Copy link
Member

Choose a reason for hiding this comment

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

LGTM, /cc @tylerwilliams who made the blake3 contribution in #18682

Copy link
Contributor

Choose a reason for hiding this comment

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

Tyler gave me the tip. I think we should do the Windows path as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah this change looks good to me too

@mauriciogg mauriciogg changed the title Disable using NEON instructions on linux arm64 Add blake3 NEON instructions on linux arm64 Aug 31, 2023
@iancha1992 iancha1992 added the team-Remote-Exec Issues and PRs for the Execution (Remote) team label Sep 1, 2023
@meteorcloudy meteorcloudy requested a review from coeuvre September 1, 2023 09:25
@meteorcloudy meteorcloudy added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Sep 1, 2023
@copybara-service copybara-service bot closed this in 31812fe Sep 4, 2023
@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Sep 4, 2023
@meteorcloudy
Copy link
Member

Can you please also sync the blake3 BUILD file change to https://github.com/bazelbuild/bazel-central-registry/tree/main/modules/blake3 ?

@meteorcloudy
Copy link
Member

meteorcloudy commented Sep 4, 2023

You can have a new version named 1.3.3.bcr.1 as a patched version. See https://github.com/bazelbuild/bazel-central-registry/blob/main/docs/README.md#versions-format

@meteorcloudy
Copy link
Member

Context: I'm working on #19087, so in future, blake3 will come from the BCR.

mauriciogg added a commit to mauriciogg/bazel-central-registry that referenced this pull request Sep 5, 2023
bazel-io pushed a commit to bazel-io/bazel that referenced this pull request Oct 12, 2023
otherwise getting libunix_jni.so: undefined symbol: blake3_hash_many_neon

see bazelbuild@d0de5e0

Closes bazelbuild#19384.

PiperOrigin-RevId: 562479599
Change-Id: I8f0ca6b68486f5ea8db698f3f99526eb1c0de8a8
keertk pushed a commit that referenced this pull request Oct 12, 2023
otherwise getting libunix_jni.so: undefined symbol:
blake3_hash_many_neon

see
d0de5e0

Closes #19384.

Commit
31812fe

PiperOrigin-RevId: 562479599
Change-Id: I8f0ca6b68486f5ea8db698f3f99526eb1c0de8a8

Co-authored-by: Mauricio G <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Remote-Exec Issues and PRs for the Execution (Remote) team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants