-
Notifications
You must be signed in to change notification settings - Fork 298
intrinsic-test
: Final code cleanup for the arm
and common
module
#1884
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
base: master
Are you sure you want to change the base?
Changes from all commits
044297a
dca2fd9
1f39d69
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
use crate::arm::intrinsic::ArmIntrinsicType; | ||
use crate::common::argument::Argument; | ||
|
||
impl Argument<ArmIntrinsicType> { | ||
pub fn type_and_name_from_c(arg: &str) -> (&str, &str) { | ||
let split_index = arg | ||
.rfind([' ', '*']) | ||
.expect("Couldn't split type and argname"); | ||
|
||
(arg[..split_index + 1].trim_end(), &arg[split_index + 1..]) | ||
} | ||
} | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,11 @@ | ||
mod argument; | ||
mod compile; | ||
mod config; | ||
mod intrinsic; | ||
mod json_parser; | ||
mod types; | ||
|
||
use std::fs::File; | ||
use std::fs::{self, File}; | ||
|
||
use rayon::prelude::*; | ||
|
||
|
@@ -69,9 +70,10 @@ impl SupportedArchitectureTest for ArmArchitectureTest { | |
|
||
let (chunk_size, chunk_count) = chunk_info(self.intrinsics.len()); | ||
|
||
let cpp_compiler = compile::build_cpp_compilation(&self.cli_options).unwrap(); | ||
let cpp_compiler_wrapped = compile::build_cpp_compilation(&self.cli_options); | ||
Comment on lines
-72
to
+73
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we really should just panic if there is no compiler configured. What are we supposed to do in that scenario? We should not fail silently. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it could be let Some(cpp_compiler) = compile::build_cpp_compilation(&self.cli_options) else {
panic!("no C compiler configured");
} I guess. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a CLI option to only generate the C and Rust test files (without building or linking). The relevant flag is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah, ok, can you add a comment mentioning that flag? (I think we should clean that up with a custom |
||
|
||
let notice = &build_notices("// "); | ||
fs::create_dir_all("c_programs").unwrap(); | ||
self.intrinsics | ||
.par_chunks(chunk_size) | ||
.enumerate() | ||
|
@@ -81,9 +83,11 @@ impl SupportedArchitectureTest for ArmArchitectureTest { | |
write_mod_cpp(&mut file, notice, c_target, platform_headers, chunk).unwrap(); | ||
|
||
// compile this cpp file into a .o file | ||
let output = cpp_compiler | ||
.compile_object_file(&format!("mod_{i}.cpp"), &format!("mod_{i}.o"))?; | ||
assert!(output.status.success(), "{output:?}"); | ||
if let Some(cpp_compiler) = cpp_compiler_wrapped.as_ref() { | ||
let output = cpp_compiler | ||
.compile_object_file(&format!("mod_{i}.cpp"), &format!("mod_{i}.o"))?; | ||
assert!(output.status.success(), "{output:?}"); | ||
} | ||
|
||
Ok(()) | ||
}) | ||
|
@@ -99,21 +103,25 @@ impl SupportedArchitectureTest for ArmArchitectureTest { | |
) | ||
.unwrap(); | ||
|
||
// compile this cpp file into a .o file | ||
info!("compiling main.cpp"); | ||
let output = cpp_compiler | ||
.compile_object_file("main.cpp", "intrinsic-test-programs.o") | ||
.unwrap(); | ||
assert!(output.status.success(), "{output:?}"); | ||
|
||
let object_files = (0..chunk_count) | ||
.map(|i| format!("mod_{i}.o")) | ||
.chain(["intrinsic-test-programs.o".to_owned()]); | ||
|
||
let output = cpp_compiler | ||
.link_executable(object_files, "intrinsic-test-programs") | ||
.unwrap(); | ||
assert!(output.status.success(), "{output:?}"); | ||
// This is done because `cpp_compiler_wrapped` is None when | ||
// the --generate-only flag is passed | ||
if let Some(cpp_compiler) = cpp_compiler_wrapped.as_ref() { | ||
// compile this cpp file into a .o file | ||
info!("compiling main.cpp"); | ||
let output = cpp_compiler | ||
.compile_object_file("main.cpp", "intrinsic-test-programs.o") | ||
.unwrap(); | ||
assert!(output.status.success(), "{output:?}"); | ||
|
||
let object_files = (0..chunk_count) | ||
.map(|i| format!("mod_{i}.o")) | ||
.chain(["intrinsic-test-programs.o".to_owned()]); | ||
|
||
let output = cpp_compiler | ||
.link_executable(object_files, "intrinsic-test-programs") | ||
.unwrap(); | ||
assert!(output.status.success(), "{output:?}"); | ||
} | ||
|
||
true | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
use std::collections::HashMap; | ||
use std::fmt; | ||
use std::ops::Deref; | ||
use std::str::FromStr; | ||
|
@@ -121,7 +122,7 @@ pub struct IntrinsicType { | |
/// A value of `None` can be assumed to be 1 though. | ||
pub vec_len: Option<u32>, | ||
|
||
pub target: String, | ||
pub metadata: HashMap<String, String>, | ||
Comment on lines
-124
to
+125
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is this a hashmap? Couldn't this be a struct instead? what values do you actually need to store in here for x86? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. X86 has a “TYPE” and “ETYPE” values for each argument of an intrinsic. The “TYPE” tag is the type of the argument/return value as it appears in the C definition, while “ETYPE” gives information about how the argument will be used (eg: when using vector arguments, whether the operation would be U64 or FP16 based) Both the information would be required, but it doesn’t make sense to create explicit struct members for it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh, I didn't see what type this was in. Hmm, shouldn't the target-specific intrinsic type hold such information? I'll say I am a bit surprised by the target being a part of |
||
} | ||
|
||
impl IntrinsicType { | ||
|
@@ -153,6 +154,10 @@ impl IntrinsicType { | |
self.ptr | ||
} | ||
|
||
pub fn set_metadata(&mut self, key: &str, value: &str) { | ||
self.metadata.insert(key.to_string(), value.to_string()); | ||
} | ||
|
||
pub fn c_scalar_type(&self) -> String { | ||
match self.kind() { | ||
TypeKind::Char(_) => String::from("char"), | ||
|
@@ -322,7 +327,7 @@ 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<Self, String> | ||
fn from_c(_s: &str) -> Result<Self, String> | ||
where | ||
Self: Sized; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this really need its own file?