Skip to content

intrinsic-test: Implemented DerefMut for ArmIntrinsicTest #1878

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 1 commit into
base: master
Choose a base branch
from

Conversation

madhav-madhusoodanan
Copy link
Contributor

@madhav-madhusoodanan madhav-madhusoodanan commented Jul 16, 2025

Summary

  1. Moved the return type of IntrinsicType::from_c to Rust<Self, String> from Result<Box, String>
  2. Implemented DerefMut for ArmIntrinsicTest struct

Context

This PR is part 2 of the changes that were originally made in the x86 extension PR #1814 for intrinsic-test, and is intended to unblock the speedup efforts that are being made in PRs #1856 , #1862 and #1863.

More details: 1. Moved the return type of IntrinsicType::from_c to
Rust<Self, String> from Result<Box<Self>, String>
@madhav-madhusoodanan madhav-madhusoodanan force-pushed the intrinsic-test-box-removing-and-more branch from 2b57ce1 to c9c18ef Compare July 16, 2025 18:19
Comment on lines +18 to +23
impl DerefMut for ArmIntrinsicType {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.0
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really worth it?

In general it is my impression that this code base is much too generic already (dyn, bunch of traits), but then only had one concrete implementation.

Deref is generally discouraged, why does it pull its weight here?

Copy link
Contributor Author

@madhav-madhusoodanan madhav-madhusoodanan Jul 17, 2025

Choose a reason for hiding this comment

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

There are 3 levels of functionality for structs that implement IntrinsicTypeDefinition:

  1. The basics (name, is_simd, is_ptr, etc) where the logic is strightforward and doesn't need to be implemented per architecture
  2. The mid level (get_load_function, c_type, etc) where the logic may vary across architectures, and would need architecture-specific definition
  3. The code generation part (generate_c_test_loop, format_c_main_template, etc) where the logic is again common across architectures:
    a. Define headers
    b. Define any typecasting functionality as per necessity
    c. Define the variable
    d. Define the list of values that would be passed as an argument to the intrinsic
    e. Define test loops
    f. Load the arguments, call the intrinsics and print the value

We figured that:

  1. Any functionality that would be needed at Level 2 could use Deref and cleanly use the functions defined on the inner type.
  2. Any functionality that would be needed at Level 3 could use Generics and associated traits to cleanly use the architecture-specific functionality

Copy link
Contributor Author

@madhav-madhusoodanan madhav-madhusoodanan Jul 17, 2025

Choose a reason for hiding this comment

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

I look forward to learning how we can better design intrinsic-test so that it supports easy addition of other architectures too.

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.

2 participants