Re-organize intrinsic-test to enable seamless addition of behaviour testing for more architectures by madhav-madhusoodanan · Pull Request #1758 · rust-lang/stdarch (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation61 Commits28 Checks62 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

@madhav-madhusoodanan

Updates so far

  1. Moved the ARM-specific testing to the arm module, exposing only a test function at src/main.rs
  2. Created a SupportedArchitectureTest trait that has been implemented for arm and should be implemented for other architectures too. Allows us to expose functionality like building C/Rust test files and comparing outputs.
  3. Moved common functionality (CLI parsing, create files, compilations, compare outputs) into the common module for reuse by other architectures too.
  4. Implemented a match block for selection of architectures using the target CLI variable.

Reasoning

  1. The Intrinsic type may be specific to architectures, hence the implementation of a trait helps us to abstract the usage of such data types within the architecture-specific module

New architecture of intrinsic-test

IMG_9057

@rustbot

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

@Amanieu

The module should be called arm rather than aarch.

@Amanieu

@JamieCunliffe feel free to have a look at this since you are the original author of intrinsic-test. This is part of a GSoC project to extend intrinsic-test to other architectures.

@madhav-madhusoodanan

Are we using the --a32 flag in the CLI of intrinsic-test anywhere? I was unable to find its usage.
Would it be alright if I remove it, or is there some reasoning that I may be missing?

@Amanieu

--a32 is not currently tested in CI, but it is used for 32-bit ARM targets (arm* and thumb*). Some ARM intrinsics are only available on 64-bit ARM targets and this flag skips those.

@madhav-madhusoodanan madhav-madhusoodanan changed the title[DRAFT] Re-organize intrinsic-test to enable seamless addition of behaviour testing for more architectures Re-organize intrinsic-test to enable seamless addition of behaviour testing for more architectures

Mar 27, 2025

@madhav-madhusoodanan madhav-madhusoodanan changed the titleRe-organize intrinsic-test to enable seamless addition of behaviour testing for more architectures [DRAFT] Re-organize intrinsic-test to enable seamless addition of behaviour testing for more architectures

Mar 28, 2025

madhav-madhusoodanan

@madhav-madhusoodanan

@madhav-madhusoodanan

Okay, I'm more or less done with resolving minor issues now.
Is this PR good enough to be merged? @adamgemmell @Amanieu ?

madhav-madhusoodanan

Comment on lines +31 to +37

pub fn write_rust_testfiles<T: IntrinsicTypeDefinition>(
intrinsics: Vec<&dyn IntrinsicDefinition>,
rust_target: &str,
notice: &str,
definitions: &str,
cfg: &str,
) -> Vec {

Choose a reason for hiding this comment

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

One change I made in the last commit was adding the "definitions" argument so that we could accommodate the f16 formatting related changes made in this commit

Amanieu
/// Defines the basic structure of achitecture-specific derivatives
/// of IntrinsicType.
#[macro_export]

Choose a reason for hiding this comment

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

Rather than using this macro, consider just implementing Deref so that methods forward to the inner type. This also removes the need for the BaseIntrinsicTypeDefinition trait, they can just be inherent methods on IntrinsicType.

Choose a reason for hiding this comment

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

Got it, thank you so much!
I was able to remove a lot of redundant code.

/// rust debug output format for the return type. The generated line assumes
/// there is an int i in scope which is the current pass number.
fn print_result_c(&self, _indentation: Indentation, _additional: &str) -> String {
unimplemented!("Architectures need to implement print_result_c!")

Choose a reason for hiding this comment

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

Why does this have a default implementation if it is mandatory?

intrinsic_call = self.name(),
args = self.arguments().as_call_param_c(),
print_result = self.print_result_c(body_indentation, additional)
)

Choose a reason for hiding this comment

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

Why is this part of the trait when it's identical to the ARM implementation? If this is intended to be the same in all architectures then it doesn't need to be in the trait. Or at least make the ARM backend us the default implementation.

Choose a reason for hiding this comment

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

In fact I think all the methods below and including this one should just be removed from the trait: there isn't really any reason for target-specific customization of these.

Choose a reason for hiding this comment

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

I thought having the common functionality in the trait would make it more ergonomic.
I overlooked the part where the specific functionality won't need to be adjusted for different architectures.

I'll update this, thank you for pointing out.

/// Determines the load function for this type.
/// can be implemented in an `impl` block
fn get_load_function(&self, _language: Language) -> String {
unimplemented!("Different architectures must implement get_load_function!")

Choose a reason for hiding this comment

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

Why provide a default implementation if it must be implemented?

Choose a reason for hiding this comment

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

Here too, I thought it might help with debugging in case the function isn't implemented downstream.
I'll remove this.

constraints
};
let indentation2 = indentation.nested();

Choose a reason for hiding this comment

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

The f16 handling in the ARM-specific code should actually be moved into the common code: f16 support is also going to be need for some x86 intrinsics.

Choose a reason for hiding this comment

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

Got it

generate_c_program(
notices,
header_files,
"aarch64",

Choose a reason for hiding this comment

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

"aarch64" in the common code.

@madhav-madhusoodanan

  1. Removed default implementation of traits that are compulsorily implemented
  2. Replaced BaseIntrinsicTypeDefinition with Deref<Target = IntrinsicType>

@madhav-madhusoodanan

@madhav-madhusoodanan

modifications) outside the IntrinsicDefinition trait

@madhav-madhusoodanan

@madhav-madhusoodanan

Over the last 4 commits, I've:

  1. Removed the default trait functionalities
    • The ones with the unimplemented!() function are removed, since they will need to be implemented in architecture-
      specific modules.
    • The ones which had common functionality are now separate functions.
  2. Renamed a bunch of functions in common/gen_c.rs and common/gen_rust.rs. There were a lot of similarly-named functions (generate_c_program, gen_c_code and the like), which are now named for better clarity like so (the same thing happens with the rust side of test-file gen too):
    • format_c_main_template
    • compile_c_programs
    • setup_c_file_paths
    • generate_c_test_loop
    • generate_c_constraint_blocks
    • create_c_test_programs
  3. Moved the f16 formatting code to the common module as a separate function

adamgemmell

Choose a reason for hiding this comment

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

Looks like a nice first step to me, thanks!

Amanieu

@Amanieu Amanieu added this pull request to the merge queue

May 27, 2025

@madhav-madhusoodanan

Thank you so much @adamgemmell @Amanieu !

There was quite a lot of learning for me on the architecture design front.

adamgemmell

.current_dir("rust_programs")
.arg("-c")
.arg(format!(
"cargo {toolchain} run --target {target} --bin {intrinsic_name} --release",

Choose a reason for hiding this comment

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

This should be run with the runner too - it's masked on CI due to the CARGO_TARGET_*_RUNNER env variable that's set in the dockerfiles

Choose a reason for hiding this comment

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

Ohh I see

bors added a commit to rust-lang/rust that referenced this pull request

Jun 7, 2025

@bors

github-actions bot pushed a commit to rust-lang/miri that referenced this pull request

Jun 8, 2025

@bors

bjorn3 pushed a commit to rust-lang/rustc_codegen_cranelift that referenced this pull request

Jun 8, 2025

@bors

github-actions bot pushed a commit to rust-lang/rustc-dev-guide that referenced this pull request

Jun 9, 2025

@bors

lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request

Jun 9, 2025

@bors

tautschnig pushed a commit to model-checking/verify-rust-std that referenced this pull request

Jun 17, 2025

@bors