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

New ARM intrinsic generator #1693

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Jamesbarford
Copy link
Contributor

New Arm intrinsic generator

A new generator for creating ARM intrinsics using YAML. The input which superseeds the old generators bespoke file format that was proving cumbersome to maintain and tricky to expand to support other architectures than neon. The YAML provides a more expressive and maintainable way to declare intrinsics.

Summary of changes

  • Remove the old generator
  • Add new generator
  • Add YAML files for generating rust instrinsics
  • aarch64/neon/generated.rs and arm_shared/neon/generated.rs are both newly created from the new input as such has a slightly different output and code style.

@rustbot
Copy link
Collaborator

rustbot commented Dec 16, 2024

r? @Amanieu

rustbot has assigned @Amanieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

adamgemmell and others added 4 commits December 18, 2024 14:21
Co-authored-by: Jamie Cunliffe <Jamie.Cunliffe@arm.com>
Co-authored-by: Jacob Bramley <jacob.bramley@arm.com>
Co-authored-by: Luca Vizzarro <Luca.Vizzarro@arm.com>
Co-authored-by: Jamie Cunliffe <Jamie.Cunliffe@arm.com>
Co-authored-by: Jacob Bramley <jacob.bramley@arm.com>
Co-authored-by: Luca Vizzarro <Luca.Vizzarro@arm.com>
@Amanieu
Copy link
Member

Amanieu commented Dec 18, 2024

Some initial notes (I'm travelling and will do a full review later):

  • I'm very glad to see this! It indeed seems much more maintainable than the old generator.
  • This should be called stdarch-gen-arm rather than stdarch-gen2.
  • We have existing manual implementations of intrinsics in crates/core_arch/src/arm_shared/neon/mod.rs, for example vbsl_p16. Ideally we should use automatic generation for all intrinsics and remove the manual implementations.

@Jamesbarford
Copy link
Contributor Author

Thanks, hope your travels are going smoothly! It can be a pretty hectic time of year.

I'll hold off changes until you've had a more thorough perusal.

With respect to the intrinsics floating about in the example you gave of crates/core_arch/src/arm_shared/neon/mod.rs. I was thinking of doing a two stage approach; replace what we are currently generating with the new generator. Then port over the other intrinsics when the opportune moment arises to do so.

This is based off it being time consuming porting the old intrinsics over coupled with the desire to unblock other units of work that cannot be achieved without the new generator.

I do, however, undoubtedly agree that the goal should be to have all of these intrinsics generated with the new generator.

crates/core_arch/src/arm_shared/neon/mod.rs Outdated Show resolved Hide resolved
crates/core_arch/src/arm_shared/neon/mod.rs Outdated Show resolved Hide resolved
crates/core_arch/src/arm_shared/neon/mod.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
crates/stdarch-verify/src/lib.rs Outdated Show resolved Hide resolved
@Jamesbarford
Copy link
Contributor Author

Thanks for your review, I believe the latest commit should have taken care of your comments.

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