Skip to content
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

Use lang tester's ignore-if to avoid encoding configuration in test names #953

Merged
merged 3 commits into from
Feb 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion tests/c/debug_debuginfo.c → tests/c/debuginfo.c
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
// ignore-if: test $YK_CARGO_PROFILE != "debug"
// Run-time:
// env-var: YKD_PRINT_IR=jit-pre-opt
// env-var: YKD_SERIALISE_COMPILATION=1
// stderr:
// ...
// define ptr @__yk_compiled_trace_0(...
// entry:
// ; main() c/debug_debuginfo.c:27:5
// ; main() c/debuginfo.c:28:5
// ...

// Check that debug information is included in module prints.
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// ignore-if: test ${YK_ARCH} != "x86_64"
// Run-time:
// env-var: YKD_SERIALISE_COMPILATION=1
// env-var: YKD_PRINT_JITSTATE=1
Expand Down
1 change: 1 addition & 0 deletions tests/c/simple.newcg.c
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// ignore-if: test YK_JIT_COMPILER != "yk"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just pick up YKD_NEW_CODEGEN here?

https://ykjit.github.io/yk/dev/runtime_config.html#ykd_new_codegen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

YKD_NEW_CODEGEN is like calling a road "new road" -- when you build another road should it be called "newer road"? The directory structure in ykrt/compiler gives these things names, so we should IMHO use those names and not a boolean NEW_CODEGEN env var.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we push a commit to rename the existing YKD_NEW_CODEGEN env var and use that instead? I don't see any benefit to having two environment variables for the same purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's probably a good idea, but the YK_ precedent is already present in the test infrastructure, so renaming YKD_NEW_CODEGEN feels like a separate PR to me (and best done by someone who knows exactly what YKD_NEW_CODEGEN does).

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. So this PR should follow precedent, and a follow up PR can do the rename.

Copy link
Contributor

Choose a reason for hiding this comment

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

(I can do the rename)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you!

// Run-time:
// env-var: YKD_PRINT_IR=aot
// env-var: YKD_SERIALISE_COMPILATION=1
Expand Down
File renamed without changes.
93 changes: 27 additions & 66 deletions tests/langtest_c.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,82 +7,16 @@ use std::{
fs::read_to_string,
path::{Path, PathBuf},
process::Command,
sync::LazyLock,
};
use tempfile::TempDir;
use tests::{mk_compiler, EXTRA_LINK};
use ykbuild::{completion_wrapper::CompletionWrapper, ykllvm_bin};

const COMMENT: &str = "//";

static NEW_CODEGEN: LazyLock<bool> = LazyLock::new(|| {
if let Ok(val) = env::var("YKD_NEW_CODEGEN") {
if val == "1" {
return true;
}
}
false
});

static SUPPORTED_CPU_ARCHES: [&str; 1] = ["x86_64"];

/// Transitionary hack to allow the LLVM and new codegen to co-exist.
///
/// Once the new codegen is complete we can kill this.
fn correct_codegen_for_test(p: &Path) -> bool {
let fname = p.file_name().unwrap().to_str().unwrap();
if *NEW_CODEGEN {
fname.contains(".newcg.")
} else {
!fname.contains(".newcg.")
}
}

/// Get string name for the current CPU architecture.
fn arch() -> &'static str {
if cfg!(target_arch = "x86_64") {
"x86_64"
} else {
panic!("unknown CPU architecture");
}
}

/// Check if we are on the right CPU for a test.
fn correct_cpu_arch(p: &Path) -> bool {
let my_arch = arch();
let other_arches = SUPPORTED_CPU_ARCHES.iter().filter(|a| *a != &my_arch);
let p = p.to_str().unwrap();
for oa in other_arches {
if p.contains(oa) {
return false;
}
}
return true;
}

fn run_suite(opt: &'static str) {
println!("Running C tests with opt level {}...", opt);

fn is_c(p: &Path) -> bool {
p.extension().as_ref().and_then(|p| p.to_str()) == Some("c")
}

// Tests with the filename prefix `debug_` are only run in debug builds.
#[cfg(cargo_profile = "release")]
let filter = |p: &Path| {
is_c(p)
&& correct_cpu_arch(p)
&& correct_codegen_for_test(p)
&& !p
.file_name()
.unwrap()
.to_str()
.unwrap()
.starts_with("debug_")
};
#[cfg(cargo_profile = "debug")]
let filter = |p: &Path| is_c(p) && correct_cpu_arch(p) && correct_codegen_for_test(p);

let tempdir = TempDir::new().unwrap();

// Generate a `compile_commands.json` database for clangd.
Expand All @@ -92,6 +26,33 @@ fn run_suite(opt: &'static str) {
}
let wrapper_path = ccg.wrapper_path();

// Set variables for tests `ignore-if`s.
#[cfg(cargo_profile = "debug")]
env::set_var("YK_CARGO_PROFILE", "debug");
#[cfg(cargo_profile = "release")]
env::set_var("YK_CARGO_PROFILE", "release");

#[cfg(target_arch = "x86_64")]
env::set_var("YK_ARCH", "x86_64");
#[cfg(not(target_arch = "x86_64"))]
panic!("Unknown target_arch");

let filter = match env::var("YKD_NEW_CODEGEN") {
Ok(x) if x == "1" => {
env::set_var("YK_JIT_COMPILER", "yk");
|p: &Path| {
// A temporary hack because at the moment virtually no tests run on the new JIT
// compiler.
p.extension().as_ref().and_then(|p| p.to_str()) == Some("c")
&& p.file_name().unwrap().to_str().unwrap().contains(".newcg")
}
}
_ => {
env::set_var("YK_JIT_COMPILER", "llvm");
|p: &Path| p.extension().as_ref().and_then(|p| p.to_str()) == Some("c")
}
};

LangTester::new()
.comment_prefix("#")
.test_dir("c")
Expand Down
8 changes: 4 additions & 4 deletions tests/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,15 @@ pub static EXTRA_LINK: LazyLock<HashMap<&'static str, Vec<ExtraLinkage>>> = Lazy
);
}
map.insert(
"x86_64_zero_len_call.c",
"pt_zero_len_call.c",
vec![ExtraLinkage::new(
"%%TEMPDIR%%/x86_64_zero_len_call.o",
"%%TEMPDIR%%/pt_zero_len_call.o",
ykllvm_bin("clang").to_owned(),
&[
"-c",
"extra_linkage/x86_64_zero_len_call.s",
"extra_linkage/pt_zero_len_call.s",
"-o",
"%%TEMPDIR%%/x86_64_zero_len_call.o",
"%%TEMPDIR%%/pt_zero_len_call.o",
],
)],
);
Expand Down