Re-organize `intrinsic-test` to enable seamless addition of behaviour testing for more architectures #1758

madhav-madhusoodanan · 2025-03-24T19:00:45Z

Updates so far

Moved the ARM-specific testing to the arm module, exposing only a test function at src/main.rs
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.
Moved common functionality (CLI parsing, create files, compilations, compare outputs) into the common module for reuse by other architectures too.
Implemented a match block for selection of architectures using the target CLI variable.

Reasoning

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`

rustbot · 2025-03-24T19:00:50Z

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 · 2025-03-25T00:00:26Z

The module should be called arm rather than aarch.

Amanieu · 2025-03-25T00:01:01Z

@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 · 2025-03-26T18:30:39Z

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 · 2025-03-27T16:00:30Z

--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.

crates/intrinsic-test/src/arm/functions.rs

…st` structs

madhav-madhusoodanan · 2025-05-21T10:27:53Z

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

madhav-madhusoodanan · 2025-05-21T10:31:26Z

crates/intrinsic-test/src/common/write_file.rs

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


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 · 2025-05-22T22:49:11Z

crates/intrinsic-test/src/common/intrinsic_helpers.rs

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


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.

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

Amanieu · 2025-05-22T22:56:08Z

crates/intrinsic-test/src/common/intrinsic.rs

+    /// 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!")


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

Amanieu · 2025-05-22T22:57:33Z

crates/intrinsic-test/src/common/intrinsic.rs

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


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.

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.

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.

Amanieu · 2025-05-22T22:58:57Z

crates/intrinsic-test/src/common/intrinsic_helpers.rs

+    /// 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!")


Why provide a default implementation if it must be implemented?

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

Amanieu · 2025-05-22T23:06:12Z

crates/intrinsic-test/src/common/intrinsic.rs

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


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.

Amanieu · 2025-05-22T23:12:04Z

crates/intrinsic-test/src/common/intrinsic.rs

+        generate_c_program(
+            notices,
+            header_files,
+            "aarch64",


"aarch64" in the common code.

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

modifications) outside the IntrinsicDefinition trait

madhav-madhusoodanan · 2025-05-27T10:10:20Z

Over the last 4 commits, I've:

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.
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
Moved the f16 formatting code to the common module as a separate function

adamgemmell

Looks like a nice first step to me, thanks!

madhav-madhusoodanan · 2025-05-28T03:36:35Z

Thank you so much @adamgemmell @Amanieu !

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

rustbot assigned Amanieu Mar 24, 2025

madhav-madhusoodanan force-pushed the restructure-intrinsic-test-crate branch 2 times, most recently from 9900e81 to 546554d Compare March 24, 2025 19:10

madhav-madhusoodanan force-pushed the restructure-intrinsic-test-crate branch 6 times, most recently from 5dce230 to 7ff8497 Compare March 26, 2025 15:32

madhav-madhusoodanan force-pushed the restructure-intrinsic-test-crate branch 2 times, most recently from a12b4a4 to ff10311 Compare March 27, 2025 15:49

madhav-madhusoodanan force-pushed the restructure-intrinsic-test-crate branch 9 times, most recently from 6e8851b to a2ce02c Compare March 27, 2025 19:19

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 marked this pull request as ready for review March 27, 2025 19:29

madhav-madhusoodanan marked this pull request as draft March 28, 2025 08:51

madhav-madhusoodanan changed the title ~~Re-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 commented Mar 28, 2025

View reviewed changes

crates/intrinsic-test/src/arm/functions.rs Outdated Show resolved Hide resolved

madhav-madhusoodanan added 7 commits May 21, 2025 13:23

moved the C compilation commands into a struct for easier handling

fb84942

Added dynamic dispatch for easier management of `<arch>ArchitectureTe…

757dd74

…st` structs

code cleanup

4935acb

chore: file renaming

c1d2d77

feat: made constraint common

90aff41

fix: aarch64_be issues wthin compilation

941637c

moved more code generation functionality to common

a4f084e

madhav-madhusoodanan force-pushed the restructure-intrinsic-test-crate branch 3 times, most recently from df2c75d to 5d360a0 Compare May 21, 2025 09:23

feat: merging changes related to f16 formatting

66a88fe

madhav-madhusoodanan force-pushed the restructure-intrinsic-test-crate branch from 5d360a0 to 66a88fe Compare May 21, 2025 09:45

madhav-madhusoodanan commented May 21, 2025

View reviewed changes

Amanieu requested changes May 22, 2025

View reviewed changes

Fix: removed BaseIntrinsicTypeDefinition + code cleanup

50fde10

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

madhav-madhusoodanan force-pushed the restructure-intrinsic-test-crate branch from 07a0221 to 50fde10 Compare May 25, 2025 15:19

madhav-madhusoodanan added 3 commits May 25, 2025 21:17

fix: moved f16 formatting code to common module

f95be81

fix: moved common code (that required no architecture-specific

cb62f84

modifications) outside the IntrinsicDefinition trait

fix: code cleanup and renaming

dfacf9d

madhav-madhusoodanan force-pushed the restructure-intrinsic-test-crate branch from 053c90a to dfacf9d Compare May 26, 2025 05:31

madhav-madhusoodanan requested review from Amanieu and adamgemmell May 27, 2025 10:10

adamgemmell approved these changes May 27, 2025

View reviewed changes

Amanieu approved these changes May 27, 2025

View reviewed changes

Amanieu added this pull request to the merge queue May 27, 2025

Merged via the queue into rust-lang:master with commit e0e82ca May 28, 2025
62 checks passed

Re-organize intrinsic-test to enable seamless addition of behaviour testing for more architectures #1758

Re-organize intrinsic-test to enable seamless addition of behaviour testing for more architectures #1758

Uh oh!

Conversation

madhav-madhusoodanan commented Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Updates so far

Reasoning

New architecture of intrinsic-test

Uh oh!

rustbot commented Mar 24, 2025

Uh oh!

Amanieu commented Mar 25, 2025

Uh oh!

Amanieu commented Mar 25, 2025

Uh oh!

madhav-madhusoodanan commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Amanieu commented Mar 27, 2025

Uh oh!

Uh oh!

madhav-madhusoodanan commented May 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

madhav-madhusoodanan commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamgemmell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

madhav-madhusoodanan commented May 28, 2025

Uh oh!

Uh oh!

Re-organize `intrinsic-test` to enable seamless addition of behaviour testing for more architectures #1758

Re-organize `intrinsic-test` to enable seamless addition of behaviour testing for more architectures #1758

madhav-madhusoodanan commented Mar 24, 2025 •

edited

Loading

New architecture of `intrinsic-test`

madhav-madhusoodanan commented Mar 26, 2025 •

edited

Loading

madhav-madhusoodanan commented May 27, 2025 •

edited

Loading