Skip to content

Fix unnecessary parse and typecheck overhead when using IR conversion check #4521

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

Open
magancarz wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix unnecessary parse and typecheck overhead when using IR conversion check #4521

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
65 changes: 30 additions & 35 deletions xls/dslx/interpreter_main.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,10 +186,10 @@ absl::StatusOr<TestResult> RealMain(
: TypeInferenceVersion::kVersion1)
: std::nullopt;

RealFilesystem vfs;
auto vfs = std::make_unique<RealFilesystem>();

XLS_ASSIGN_OR_RETURN(std::string program,
vfs.GetFileContents(entry_module_path));
vfs->GetFileContents(entry_module_path));
XLS_ASSIGN_OR_RETURN(std::string module_name, PathToName(entry_module_path));

std::unique_ptr<AbstractRunComparator> run_comparator;
Expand Down Expand Up @@ -268,10 +268,17 @@ absl::StatusOr<TestResult> RealMain(
? &results_proto
: nullptr;

ImportData import_data =
CreateImportData(parse_and_typecheck_options.dslx_stdlib_path,
parse_and_typecheck_options.dslx_paths,
parse_and_typecheck_options.warnings, std::move(vfs));
std::unique_ptr<AbstractTestRunner> test_runner = GetTestRunner(evaluator);
XLS_ASSIGN_OR_RETURN(TestResultData test_result,
XLS_ASSIGN_OR_RETURN(ParseAndTestResult parse_and_test_result,
test_runner->ParseAndTest(program, module_name,
entry_module_path, options));
entry_module_path, options,
import_data));
TestResultData& test_result = parse_and_test_result.test_result;
TypecheckedModule& tm = parse_and_test_result.typechecked_module;

if (xml_output_file.has_value()) {
test_xml::TestSuites suites = test_result.ToXmlSuites(module_name);
Expand All @@ -292,58 +299,46 @@ absl::StatusOr<TestResult> RealMain(
// Early feeback if the code cannot be lowered to IR.
std::optional<bool> lower_to_ir_flag = absl::GetFlag(FLAGS_lower_to_ir);
if (lower_to_ir_flag.value_or(false)) {
LOG(INFO) << "Checking if code can be lowered to IR";
std::optional<bool> convert_tests = absl::GetFlag(FLAGS_convert_tests);
bool is_convert_tests = convert_tests.value_or(false);
bool is_type_inference_v2 = type_inference_v2_flag.value_or(false);
bool printed_error = true;

ConvertOptions ir_convert_options = {
.emit_positions = true,
.emit_assert = true,
.emit_cover = true,
.verify_ir = true,
.warnings_as_errors = false,
.warnings = kAllWarningsSet,
.convert_tests = is_convert_tests,
.type_inference_v2 = is_type_inference_v2,
.lower_to_proc_scoped_channels = true,
};
std::array<std::string_view, 1> module_path{entry_module_path};

ImportData import_data(CreateImportData(
dslx_stdlib_path.string(), dslx_paths, ir_convert_options.warnings,
std::make_unique<RealFilesystem>()));

absl::StatusOr<TypecheckedModule> tm = ParseAndTypecheck(
program, entry_module_path, module_name, &import_data);
LOG(INFO) << absl::StrFormat("Checking if %s can be lowered to IR",
module_name.c_str());

// Module conversion cannot be used because it skips CheckAcceptableTopProc.
// Instead, we collect non-parametric processes and functions which are then
// passed separately as tops.
std::vector<std::string> module_elements;

std::vector<Proc*> module_procs = tm->module->GetProcs();
std::vector<Proc*> module_procs = tm.module->GetProcs();
for (Proc* elem : module_procs) {
if (!elem->IsParametric()) {
module_elements.push_back(elem->identifier());
}
}

std::vector<Function*> module_funcs = tm->module->GetFunctions();
std::vector<Function*> module_funcs = tm.module->GetFunctions();
for (Function* elem : module_funcs) {
if (!elem->IsParametric()) {
module_elements.push_back(elem->identifier());
}
}

ConvertOptions ir_convert_options = {
.emit_positions = true,
.emit_assert = true,
.emit_cover = true,
.verify_ir = true,
.warnings_as_errors = false,
.warnings = kAllWarningsSet,
.convert_tests = absl::GetFlag(FLAGS_convert_tests).value_or(false),
.type_inference_v2 = type_inference_v2_flag.value_or(false),
.lower_to_proc_scoped_channels = true,
};
ParametricEnv stub_parametric_env;
std::vector<std::string> failed_ir_conversion_entries;
for (std::string& elem : module_elements) {
// Convert to IR each element separately.
absl::StatusOr<PackageConversionData> ir_conv_result =
ConvertFilesToPackage(module_path, dslx_stdlib_path.string(),
dslx_paths, ir_convert_options, elem,
module_name, &printed_error);
absl::StatusOr<std::string> ir_conv_result =
ConvertOneFunction(tm.module, elem, &import_data,
Comment thread
magancarz marked this conversation as resolved.
&stub_parametric_env, ir_convert_options);
if (!ir_conv_result.ok()) {
failed_ir_conversion_entries.push_back(std::move(elem));
}
Expand Down
41 changes: 28 additions & 13 deletions xls/dslx/run_routines/run_routines.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -936,9 +936,6 @@ absl::StatusOr<ParseAndProveResult> ParseAndProve(
absl::StatusOr<TestResultData> AbstractTestRunner::ParseAndTest(
std::string_view program, std::string_view module_name,
std::string_view filename, const ParseAndTestOptions& options) const {
const absl::Time start = absl::Now();
TestResultData result(start, /*test_cases=*/{});

std::unique_ptr<VirtualizableFilesystem> vfs;
if (options.vfs_factory != nullptr) {
vfs = options.vfs_factory();
Expand All @@ -947,12 +944,25 @@ absl::StatusOr<TestResultData> AbstractTestRunner::ParseAndTest(
}
const ParseAndTypecheckOptions& parse_and_typecheck_options =
options.parse_and_typecheck_options;
auto import_data =
ImportData import_data =
CreateImportData(parse_and_typecheck_options.dslx_stdlib_path,
parse_and_typecheck_options.dslx_paths,
parse_and_typecheck_options.warnings, std::move(vfs));
FileTable& file_table = import_data.file_table();
XLS_ASSIGN_OR_RETURN(ParseAndTestResult result,
ParseAndTest(program, module_name, filename,
options, import_data));
return result.test_result;
}

absl::StatusOr<ParseAndTestResult> AbstractTestRunner::ParseAndTest(
std::string_view program, std::string_view module_name,
std::string_view filename, const ParseAndTestOptions& options,
ImportData& import_data) const {
const absl::Time start = absl::Now();
TestResultData result(start, /*test_cases=*/{});

const ParseAndTypecheckOptions& parse_and_typecheck_options =
options.parse_and_typecheck_options;
absl::StatusOr<TypecheckedModule> tm = ParseAndTypecheck(
program, filename, module_name, &import_data, nullptr,
ConvertOptions{.configured_values =
Expand All @@ -961,7 +971,11 @@ absl::StatusOr<TestResultData> AbstractTestRunner::ParseAndTest(
if (TryPrintError(tm.status(), import_data.file_table(),
import_data.vfs())) {
result.Finish(TestResult::kParseOrTypecheckError, absl::Now() - start);
return result;
return ParseAndTestResult{
result,
TypecheckedModule{
/*module=*/nullptr, /*type_info=*/nullptr,
WarningCollector{parse_and_typecheck_options.warnings}}};
}
return tm.status();
}
Expand All @@ -977,13 +991,13 @@ absl::StatusOr<TestResultData> AbstractTestRunner::ParseAndTest(
if (parse_and_typecheck_options.warnings_as_errors &&
!tm->warnings.warnings().empty()) {
result.Finish(TestResult::kFailedWarnings, absl::Now() - start);
return result;
return ParseAndTestResult{result, std::move(*tm)};
}

// If not executing tests and quickchecks, then return vacuous success.
if (!options.execute) {
result.Finish(TestResult::kAllPassed, absl::Now() - start);
return result;
return ParseAndTestResult{result, std::move(*tm)};
}

Module* entry_module = tm->module;
Expand All @@ -1001,7 +1015,7 @@ absl::StatusOr<TestResultData> AbstractTestRunner::ParseAndTest(
if (TryPrintError(ir_package_conversion_data.status(),
import_data.file_table(), import_data.vfs())) {
result.Finish(TestResult::kSomeFailed, absl::Now() - start);
return result;
return ParseAndTestResult{result, std::move(*tm)};
}
return xabsl::StatusBuilder(ir_package_conversion_data.status())
<< "Failed to convert input to IR for comparison. Consider "
Expand Down Expand Up @@ -1038,7 +1052,7 @@ absl::StatusOr<TestResultData> AbstractTestRunner::ParseAndTest(
auto test_case_end = absl::Now();
result.AddTestCase(test_xml::TestCase{
.name = test_name,
.file = std::string{start_pos.GetFilename(file_table)},
.file = std::string{start_pos.GetFilename(import_data.file_table())},
.line = start_pos.GetHumanLineno(),
.status = test_xml::RunStatus::kRun,
.result = test_xml::RunResult::kFiltered,
Expand Down Expand Up @@ -1090,7 +1104,7 @@ absl::StatusOr<TestResultData> AbstractTestRunner::ParseAndTest(
// Add to the tracking data.
result.AddTestCase(test_xml::TestCase{
.name = test_name,
.file = std::string{start_pos.GetFilename(file_table)},
.file = std::string{start_pos.GetFilename(import_data.file_table())},
.line = start_pos.GetHumanLineno(),
.status = test_xml::RunStatus::kRun,
.result = test_xml::RunResult::kCompleted,
Expand All @@ -1105,7 +1119,8 @@ absl::StatusOr<TestResultData> AbstractTestRunner::ParseAndTest(
}
HandleError(result, out.result, test_name, start_pos, test_case_start,
test_case_end - test_case_start,
/*is_quickcheck=*/false, file_table, import_data.vfs());
/*is_quickcheck=*/false, import_data.file_table(),
import_data.vfs());
}
}

Expand All @@ -1126,7 +1141,7 @@ absl::StatusOr<TestResultData> AbstractTestRunner::ParseAndTest(
result.Finish(
result.DidAnyFail() ? TestResult::kSomeFailed : TestResult::kAllPassed,
absl::Now() - start);
return result;
return ParseAndTestResult{result, std::move(*tm)};
}

std::string_view TestResultToString(TestResult tr) {
Expand Down
11 changes: 11 additions & 0 deletions xls/dslx/run_routines/run_routines.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,11 @@ class TestResultData {

class AbstractParsedTestRunner;

struct ParseAndTestResult {
TestResultData test_result;
TypecheckedModule typechecked_module;
};

class AbstractTestRunner {
public:
virtual ~AbstractTestRunner() = default;
Expand All @@ -224,6 +229,12 @@ class AbstractTestRunner {
std::string_view program, std::string_view module_name,
std::string_view filename, const ParseAndTestOptions& options) const;

// Same as above but also returns result of module parsing and typechecking.
absl::StatusOr<ParseAndTestResult> ParseAndTest(
std::string_view program, std::string_view module_name,
std::string_view filename, const ParseAndTestOptions& options,
ImportData& import_data) const;

protected:
virtual absl::StatusOr<std::unique_ptr<AbstractParsedTestRunner>>
CreateTestRunner(ImportData* import_data, TypeInfo* type_info, Module* module,
Expand Down