Skip to content

Add arm_intrinsics.json for intrinsic-test and stdarch-verify #1427

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 2 commits into from
May 15, 2023

Conversation

adamgemmell
Copy link
Contributor

This removes the ACLE submodule and ARM HTML scrape, resolving a few licensing issues. The JSON file, along with updates to QEMU and Clang in CI also increases the test coverage of Neon intrinsics.

cc: rust-lang/rust#90972 @pietroalbini

This involves moving from the ACLE intrinsic definitions (which aren't
available for SVE at this point) to a JSON file. This was derived from
ARM's documentation[^1], and then relicensed under `MIT OR Apache-2.0` for
use in this repository.

[^1]: https://developer.arm.com/architectures/instruction-sets/intrinsics
@rustbot
Copy link
Collaborator

rustbot commented May 11, 2023

r? @Amanieu

(rustbot has picked a reviewer for you, use r? to override)

#vqshluq_n_s32
#vqshluq_n_s64
#vqshluq_n_s8
#vqshlus_n_s32
Copy link
Member

Choose a reason for hiding this comment

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

My understanding is that this issue is still present if intrinsic-test is changed to run the Rust code without --release. The problem is due to the way we generate LLVM IR: we emit the vector argument as a constant and then load it, but LLVM's intrinsic only works if the vector is passed as an immediate to the intrinsics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's still there - hence why I've only commented these intrinsics out instead of removing them. We have a ticket to fix this in the future. The test tool currently runs in --release mode and I think it makes sense to extend coverage by including this set.

vmul_lane_f32
vmul_n_f32
vmulq_lane_f32
vmulq_n_f32
Copy link
Member

Choose a reason for hiding this comment

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

Have you checked this by trying the C version without the -O2 flag?

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, everything passes on aarch64 and armv7 with Rust --release and clang -O0

@Amanieu Amanieu merged commit 865c805 into rust-lang:master May 15, 2023
@Amanieu
Copy link
Member

Amanieu commented May 15, 2023

Thanks!

@adamgemmell
Copy link
Contributor Author

@Amanieu I've made a mistake and was about to push a fix that would fix CI! Should I open a new PR?

@Amanieu
Copy link
Member

Amanieu commented May 15, 2023

Yes, please do!

@pietroalbini
Copy link
Member

Thanks ❤️

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants