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.

Sign up for GitHub

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
+19 −12
Draft

intrinsic-test: Implemented DerefMut for ArmIntrinsicTest #1878

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion crates/intrinsic-test/src/arm/intrinsic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use crate::common::argument::ArgumentList;
use crate::common::indentation::Indentation;
use crate::common::intrinsic::{Intrinsic, IntrinsicDefinition};
use crate::common::intrinsic_helpers::{IntrinsicType, IntrinsicTypeDefinition, Sign, TypeKind};
use std::ops::Deref;
use std::ops::{Deref, DerefMut};

#[derive(Debug, Clone, PartialEq)]
pub struct ArmIntrinsicType(pub IntrinsicType);
Expand All @@ -15,6 +15,12 @@ impl Deref for ArmIntrinsicType {
}
}

impl DerefMut for ArmIntrinsicType {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.0
}
}

Comment on lines +18 to +23
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 •
edited
Loading

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 •
edited
Loading

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.

impl IntrinsicDefinition<ArmIntrinsicType> for Intrinsic<ArmIntrinsicType> {
fn arguments(&self) -> ArgumentList<ArmIntrinsicType> {
self.arguments.clone()
Expand Down
2 changes: 1 addition & 1 deletion crates/intrinsic-test/src/arm/json_parser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ fn json_to_intrinsic(
Ok(Intrinsic {
name,
arguments,
results: *results,
results: results,
arch_tags: intr.architectures,
})
}
Expand Down
15 changes: 7 additions & 8 deletions crates/intrinsic-test/src/arm/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ impl IntrinsicTypeDefinition for ArmIntrinsicType {
}
}

fn from_c(s: &str, target: &str) -> Result<Box<Self>, String> {
fn from_c(s: &str, target: &str) -> Result<Self, String> {
const CONST_STR: &str = "const";
if let Some(s) = s.strip_suffix('*') {
let (s, constant) = match s.trim().strip_suffix(CONST_STR) {
Expand All @@ -131,9 +131,8 @@ impl IntrinsicTypeDefinition for ArmIntrinsicType {
let s = s.trim_end();
let temp_return = ArmIntrinsicType::from_c(s, target);
temp_return.map(|mut op| {
let edited = op.as_mut();
edited.0.ptr = true;
edited.0.ptr_constant = constant;
op.ptr = true;
op.ptr_constant = constant;
op
})
} else {
Expand Down Expand Up @@ -163,7 +162,7 @@ impl IntrinsicTypeDefinition for ArmIntrinsicType {
),
None => None,
};
Ok(Box::new(ArmIntrinsicType(IntrinsicType {
Ok(ArmIntrinsicType(IntrinsicType {
ptr: false,
ptr_constant: false,
constant,
Expand All @@ -172,14 +171,14 @@ impl IntrinsicTypeDefinition for ArmIntrinsicType {
simd_len,
vec_len,
target: target.to_string(),
})))
}))
} else {
let kind = start.parse::<TypeKind>()?;
let bit_len = match kind {
TypeKind::Int(_) => Some(32),
_ => None,
};
Ok(Box::new(ArmIntrinsicType(IntrinsicType {
Ok(ArmIntrinsicType(IntrinsicType {
ptr: false,
ptr_constant: false,
constant,
Expand All @@ -188,7 +187,7 @@ impl IntrinsicTypeDefinition for ArmIntrinsicType {
simd_len: None,
vec_len: None,
target: target.to_string(),
})))
}))
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion crates/intrinsic-test/src/common/argument.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ where
Argument {
pos,
name: String::from(var_name),
ty: *ty,
ty: ty,
constraint,
}
}
Expand Down
4 changes: 3 additions & 1 deletion crates/intrinsic-test/src/common/intrinsic_helpers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,9 @@ pub trait IntrinsicTypeDefinition: Deref<Target = IntrinsicType> {
fn get_lane_function(&self) -> String;

/// can be implemented in an `impl` block
fn from_c(_s: &str, _target: &str) -> Result<Box<Self>, String>;
fn from_c(_s: &str, _target: &str) -> Result<Self, String>
where
Self: Sized;

/// Gets a string containing the typename for this type in C format.
/// can be directly defined in `impl` blocks
Expand Down