Skip to content

Adding the x86 part of behavioural testing for std::arch intrinsics #1814

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

Draft
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

madhav-madhusoodanan
Copy link
Contributor

No description provided.

@madhav-madhusoodanan madhav-madhusoodanan force-pushed the x86_extension_intrinsic_test branch 3 times, most recently from b2777ac to 7047369 Compare June 8, 2025 17:19
Comment on lines 49 to 60
match self {
Self::BFloat => "bfloat",
Self::Float => "float",
Self::Int => "int",
Self::UInt => "uint",
Self::Double => "double",
Self::Int(true) => "int",
Self::Int(false) => "uint",
Self::Poly => "poly",
Self::Void => "void",
Self::Char(true) => "char",
Self::Char(false) => "unsigned char",
Self::Short(true) => "short",
Self::Short(false) => "unsigned short",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Amanieu @adamgemmell Are we using this trait (fmt::Display for TypeKind) anywhere in the crate? I was searching for its usage and I was unable to find any.

If we are not using this trait, may I delete the definition?

Copy link
Member

Choose a reason for hiding this comment

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

I suspect this may be useful for debug output during development. However since it isn't used for functionality then it's fine to make any changes you want to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it.

@madhav-madhusoodanan
Copy link
Contributor Author

Pardon the git history, I will be sorting that out shortly

@madhav-madhusoodanan madhav-madhusoodanan force-pushed the x86_extension_intrinsic_test branch 5 times, most recently from ed8dfde to e6ff768 Compare June 16, 2025 08:28
UInt,

// if signed, then the inner value is true
Int(bool),
Copy link
Member

Choose a reason for hiding this comment

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

Rather than using a bool, considering instead something like:

enum Signed {
    Signed,
    Unsigned,
}

This ends up making the code much more readable.

format!(
"\
{line_prefix}This is a transient test file, not intended for distribution. Some aspects of the
{line_prefix}test are derived from a JSON specification, published under the same license as the
Copy link
Member

Choose a reason for hiding this comment

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

x86 uses an XML specification, not JSON.

@madhav-madhusoodanan madhav-madhusoodanan force-pushed the x86_extension_intrinsic_test branch 2 times, most recently from e113a61 to bc5da7d Compare June 19, 2025 06:52
@madhav-madhusoodanan
Copy link
Contributor Author

The CI is giving me an odd error, even after I rebased from master:

  Updating crates.io index
     Locking 5 packages to latest compatible versions
      Adding quick-xml v0.37.5
      Adding serde-xml-rs v0.8.1
      Adding thiserror v1.0.69
      Adding thiserror-impl v1.0.69
      Adding xml-rs v0.8.26
   Compiling core_arch v0.1.5 (/Users/runner/work/stdarch/stdarch/crates/core_arch)
error: the feature `generic_arg_infer` has been stable since 1.89.0-nightly and no longer requires an attribute to enable
  --> crates/core_arch/src/lib.rs:34:5
   |
34 |     generic_arg_infer,
   |     ^^^^^^^^^^^^^^^^^
   |
   = note: `-D stable-features` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(stable_features)]`

@madhav-madhusoodanan madhav-madhusoodanan force-pushed the x86_extension_intrinsic_test branch from bc5da7d to c5dec86 Compare June 19, 2025 11:24
@Amanieu
Copy link
Member

Amanieu commented Jun 19, 2025

The CI is giving me an odd error, even after I rebased from master:

  Updating crates.io index
     Locking 5 packages to latest compatible versions
      Adding quick-xml v0.37.5
      Adding serde-xml-rs v0.8.1
      Adding thiserror v1.0.69
      Adding thiserror-impl v1.0.69
      Adding xml-rs v0.8.26
   Compiling core_arch v0.1.5 (/Users/runner/work/stdarch/stdarch/crates/core_arch)
error: the feature `generic_arg_infer` has been stable since 1.89.0-nightly and no longer requires an attribute to enable
  --> crates/core_arch/src/lib.rs:34:5
   |
34 |     generic_arg_infer,
   |     ^^^^^^^^^^^^^^^^^
   |
   = note: `-D stable-features` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(stable_features)]`

Yes, this regularly happens due to changes in nightly Rust. In this case you can just remove the feature from lib.rs.

1. filtered any intrinsic arguments where the only parameter is of type
"void"
2. Better parsing for intrinsic return type
3. Filtering intrinsics that are difficult to test (void returns)
4. Reduced the variants in TypeKind (which differed only in bit_len)
necessary.

Further notes:
1. IntrinsicType.target -> IntrinsicType.metadata
2. Maintaining a HashMap to support arbitrary data storage for other
arch
3. Create a dedicated "from_param" associated function for X86IntrinsicType
@madhav-madhusoodanan madhav-madhusoodanan force-pushed the x86_extension_intrinsic_test branch 2 times, most recently from 9c953a7 to c6491da Compare June 20, 2025 05:38
…ectly instead of

constructing it.

Further details:
1. moved "type_and_name_from_c" from Argument<T> to
Argument<ArmIntrinsicType>
2. removed "from_c" in Argument<T>
@madhav-madhusoodanan madhav-madhusoodanan force-pushed the x86_extension_intrinsic_test branch from c6491da to d0f2d78 Compare June 20, 2025 06:31
@madhav-madhusoodanan madhav-madhusoodanan force-pushed the x86_extension_intrinsic_test branch from 5817342 to 5b89089 Compare July 10, 2025 06:53
@folkertdev
Copy link
Contributor

Hi, cool project!

I'm currently investigating how we can speed up the arm/aarch64 version of intrinsic-test. The CI job with it currently takes 28 minutes, and that is just not acceptible; it slows everything/everyone down.

On the other hand, I don't want to create horrible merge conflicts for you. The two main changes that I want to make are

  • combine tests into larger files (one per core), this doesn't really change anything relevant to you
  • write into a &mut impl std::io::Write instead of concatenating strings

The latter change mostly just helps with making the former possible. In the experimental #1856 I've now gotten CI time down to 8 minutes.

So, to prevent merge conflicts (especially now that we have additional reviewing capacity) I'd encourage you to split out and merge changes early if they mostly touch the existing code, instead of accumulating them in one big PR. Some changes that look like they can be split out are:

  • removing some of the Box wrapping (e.g. some of the changes in crates/intrinsic-test/src/arm/types.rs)
  • the pub enum Sign and its use in TypeKind::Int
  • the cleanup in crates/intrinsic-test/src/common/gen_c.rs

There might be others that I missed.

Feel free to r? @folkertdev me on any such cleanup PRs.

@madhav-madhusoodanan
Copy link
Contributor Author

madhav-madhusoodanan commented Jul 10, 2025

Hi @folkertdev, thank you so much for the suggestion and the concern about merge conflicts (and about looking into how we can optimize compile times!)
I was checking out #1856 and I agree with you on the need for the same.

I shall create small PRs with the changes that needs to be made on the ARM and the common modules, once I reconcile the changes with this PR itself.

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.

3 participants