Skip to content

Commit

Permalink
Fix use-after-free issues with benchmark_main.
Browse files Browse the repository at this point in the history 
The critical-path and schedule data structures contain pointers into the IR. Transformation of the IR in the scheduling and codegen pipelines invalidates the IR.

PiperOrigin-RevId: 683339164
  • Loading branch information
@meheffernan @copybara-github
meheffernan authored and copybara-github committed Oct 7, 2024
1 parent 47b1b8b commit f9f929b
Show file tree
Hide file tree
Showing 3 changed files with 224 additions and 112 deletions.
112 changes: 64 additions & 48 deletions xls/dev_tools/benchmark_main.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -722,6 +722,34 @@ absl::Status RunInterpreterAndJit(FunctionBase* function_base,
return RunProcInterpreterAndJit(proc, description, rng_engine);
}

absl::Status AnalyzeAndPrintCriticalPath(
FunctionBase* f, std::optional<int64_t> effective_clock_period_ps,
const DelayEstimator& delay_estimator, const QueryEngine& query_engine,
PipelineScheduleOrGroup* schedules, synthesis::Synthesizer* synthesizer) {
XLS_ASSIGN_OR_RETURN(
std::vector<CriticalPathEntry> critical_path,
AnalyzeCriticalPath(f, effective_clock_period_ps, delay_estimator));
synthesis::SynthesizedDelayDiffByStage delay_diff;
if (synthesizer) {
if (schedules) {
XLS_ASSIGN_OR_RETURN(
delay_diff,
CreateDelayDiffByStage(f, std::get<PipelineSchedule>(*schedules),
delay_estimator, synthesizer));
delay_diff.total_diff.critical_path = std::move(critical_path);
} else {
XLS_ASSIGN_OR_RETURN(
delay_diff.total_diff,
SynthesizeAndGetDelayDiff(f, critical_path, synthesizer));
}
} else {
delay_diff.total_diff.critical_path = std::move(critical_path);
}
XLS_RETURN_IF_ERROR(PrintCriticalPath(f, query_engine, delay_diff));
XLS_RETURN_IF_ERROR(PrintTotalDelay(f, delay_estimator));
return absl::OkStatus();
}

absl::Status RealMain(std::string_view path) {
VLOG(1) << "Reading contents at path: " << path;
XLS_ASSIGN_OR_RETURN(std::string contents, GetFileContents(path));
Expand Down Expand Up @@ -778,9 +806,6 @@ absl::Status RealMain(std::string_view path) {
GetDelayEstimator(scheduling_options_flags_proto.delay_model()));
}
const auto& delay_estimator = *pdelay_estimator;
XLS_ASSIGN_OR_RETURN(
std::vector<CriticalPathEntry> critical_path,
AnalyzeCriticalPath(f, effective_clock_period_ps, delay_estimator));
std::unique_ptr<synthesis::Synthesizer> synthesizer;
if (absl::GetFlag(FLAGS_compare_delay_to_synthesis)) {
synthesis::GrpcSynthesizerParameters parameters(
Expand All @@ -797,62 +822,53 @@ absl::Status RealMain(std::string_view path) {
scheduling_options_flags_proto.clock_period_ps() > 0 ||
scheduling_options_flags_proto.pipeline_stages() > 0;
if (!f->IsProc() && !benchmark_codegen) {
synthesis::SynthesizedDelayDiff delay_diff;
if (synthesizer) {
XLS_ASSIGN_OR_RETURN(
delay_diff,
SynthesizeAndGetDelayDiff(f, critical_path, synthesizer.get()));
} else {
delay_diff.critical_path = std::move(critical_path);
}
XLS_RETURN_IF_ERROR(PrintCriticalPath(
f, query_engine, {.total_diff = std::move(delay_diff)}));
XLS_RETURN_IF_ERROR(PrintTotalDelay(f, delay_estimator));
XLS_RETURN_IF_ERROR(AnalyzeAndPrintCriticalPath(
f, effective_clock_period_ps, delay_estimator, query_engine,
/*schedules=*/nullptr, synthesizer.get()));
} else if (benchmark_codegen) {
TimingReport timing_report;
PipelineScheduleOrGroup schedules = PackagePipelineSchedules();
XLS_ASSIGN_OR_RETURN(
CodegenResult codegen_result,
ScheduleAndCodegen(package.get(), scheduling_options_flags_proto,
codegen_flags_proto, delay_model_flag_passed,
&timing_report, &schedules));
synthesis::SynthesizedDelayDiffByStage delay_diff;
if (synthesizer) {
XLS_ASSIGN_OR_RETURN(
delay_diff,
CreateDelayDiffByStage(f, std::get<PipelineSchedule>(schedules),
*pdelay_estimator, synthesizer.get()));
PipelineScheduleOrGroup* schedules_ptr = nullptr;
if (codegen_flags_proto.generator() == GENERATOR_KIND_PIPELINE) {
XLS_ASSIGN_OR_RETURN(SchedulingOptions scheduling_options,
SetUpSchedulingOptions(
scheduling_options_flags_proto, package.get()));
absl::Duration scheduling_time;
XLS_ASSIGN_OR_RETURN(schedules,
Schedule(package.get(), scheduling_options,
&delay_estimator, &scheduling_time));
std::cout << absl::StreamFormat("Scheduling time: %dms\n",
scheduling_time / absl::Milliseconds(1));
XLS_RETURN_IF_ERROR(AnalyzeAndPrintCriticalPath(
f, effective_clock_period_ps, delay_estimator, query_engine,
&schedules, synthesizer.get()));
schedules_ptr = &schedules;

// Scheduling can change the nodes in f slightly so we need to recompute
// the bdd.
BddQueryEngine sched_qe(BddFunction::kDefaultPathLimit);
XLS_RETURN_IF_ERROR(sched_qe.Populate(f).status());
XLS_RETURN_IF_ERROR(PrintScheduleInfo(
f, schedules, sched_qe, delay_estimator,
scheduling_options_flags_proto.has_clock_period_ps()
? std::make_optional(
scheduling_options_flags_proto.clock_period_ps())
: std::nullopt));
}
delay_diff.total_diff.critical_path = std::move(critical_path);
XLS_RETURN_IF_ERROR(PrintCriticalPath(f, query_engine, delay_diff));
XLS_RETURN_IF_ERROR(PrintTotalDelay(f, delay_estimator));
absl::Duration codegen_time;
XLS_ASSIGN_OR_RETURN(CodegenResult codegen_result,
Codegen(package.get(), scheduling_options_flags_proto,
codegen_flags_proto, delay_model_flag_passed,
&schedules, &codegen_time));
std::cout << absl::StreamFormat("Codegen time: %dms\n",
codegen_time / absl::Milliseconds(1));

std::cout << absl::StreamFormat(
"Scheduling time: %dms\n",
timing_report.scheduling_time / absl::Milliseconds(1));
std::cout << absl::StreamFormat(
"Codegen time: %dms\n",
timing_report.codegen_time / absl::Milliseconds(1));

// TODO(meheff): Add an estimate of total number of gates.
std::cout << absl::StreamFormat(
"Lines of Verilog: %d\n",
std::vector<std::string>(
absl::StrSplit(codegen_result.module_generator_result.verilog_text,
'\n'))
.size());

// Scheduling can change the nodes in f slightly so we need to recompute the
// bdd.
BddQueryEngine sched_qe(BddFunction::kDefaultPathLimit);
XLS_RETURN_IF_ERROR(sched_qe.Populate(f).status());
XLS_RETURN_IF_ERROR(PrintScheduleInfo(
f, schedules, sched_qe, delay_estimator,
scheduling_options_flags_proto.has_clock_period_ps()
? std::make_optional(
scheduling_options_flags_proto.clock_period_ps())
: std::nullopt));

// Print out state information for procs.
if (f->IsProc()) {
XLS_RETURN_IF_ERROR(PrintProcInfo(f->AsProcOrDie()));
Expand Down
210 changes: 148 additions & 62 deletions xls/tools/codegen.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,110 @@
namespace xls {
namespace {

// Set the top of the package from the given codegen flags. Returns an error if
// the top is not set either by the flags or by the IR initially.
absl::Status MaybeSetTop(Package* p,
const CodegenFlagsProto& codegen_flags_proto) {
if (!codegen_flags_proto.top().empty()) {
XLS_RETURN_IF_ERROR(p->SetTopByName(codegen_flags_proto.top()));
}
XLS_RET_CHECK(p->GetTop().has_value())
<< "Package " << p->name() << " needs a top function/proc.";
return absl::OkStatus();
}

// Data structure holding all the options and metadata for codegen and
// scheduling. This is complicated because codegen options depend on scheduling
// flags and scheduling options depend on codegen flags, plus multiple delay
// estimators are required which contain references to each other. This data
// structure puts them all in one place so the logic can be reused.
struct CodegenMetadata {
SchedulingOptions scheduling_options;
verilog::CodegenOptions codegen_options;
const DelayEstimator* base_estimator = nullptr;
std::unique_ptr<FfiDelayEstimator> ffi_estimator;
std::unique_ptr<FirstMatchDelayEstimator> first_match_estimator;
DelayEstimator* delay_estimator = nullptr;

static absl::StatusOr<CodegenMetadata> Create(
Package* package,
const SchedulingOptionsFlagsProto& scheduling_options_flags_proto,
const CodegenFlagsProto& codegen_flags_proto, bool with_delay_model) {
CodegenMetadata metadata;
XLS_ASSIGN_OR_RETURN(metadata.codegen_options,
CodegenOptionsFromProto(codegen_flags_proto));

if (codegen_flags_proto.generator() == GENERATOR_KIND_COMBINATIONAL) {
if (with_delay_model) {
XLS_ASSIGN_OR_RETURN(
metadata.delay_estimator,
SetUpDelayEstimator(scheduling_options_flags_proto));
}
return metadata;
}

// Note: this should already be validated by CodegenFlagsFromAbslFlags().
CHECK_EQ(codegen_flags_proto.generator(), GENERATOR_KIND_PIPELINE)
<< "Invalid generator kind: "
<< static_cast<int>(codegen_flags_proto.generator());

XLS_ASSIGN_OR_RETURN(
metadata.scheduling_options,
SetUpSchedulingOptions(scheduling_options_flags_proto, package));

QCHECK(metadata.scheduling_options.pipeline_stages() != 0 ||
metadata.scheduling_options.clock_period_ps() != 0)
<< "Must specify --pipeline_stages or --clock_period_ps (or both).";

// Add IO constraints for RAMs.
for (const std::unique_ptr<xls::verilog::RamConfiguration>& ram_config :
metadata.codegen_options.ram_configurations()) {
for (const IOConstraint& ram_constraint :
ram_config->GetIOConstraints()) {
metadata.scheduling_options.add_constraint(ram_constraint);
}
}

XLS_ASSIGN_OR_RETURN(metadata.base_estimator,
SetUpDelayEstimator(scheduling_options_flags_proto));
metadata.ffi_estimator = std::make_unique<FfiDelayEstimator>(
metadata.scheduling_options.ffi_fallback_delay_ps());
metadata.first_match_estimator = std::make_unique<FirstMatchDelayEstimator>(
"combined_estimator",
std::vector<const DelayEstimator*>(
{metadata.base_estimator, metadata.ffi_estimator.get()}));
metadata.delay_estimator = metadata.first_match_estimator.get();

if (package->GetTop().value()->IsProc()) {
// Force using non-pretty printed codegen when generating procs.
// TODO(tedhong): 2021-09-25 - Update pretty-printer to support
// blocks with flow control.
metadata.codegen_options.emit_as_pipeline(false);
}
return metadata;
}
};

absl::StatusOr<PipelineScheduleOrGroup> ScheduleFromMetadata(
Package* p, const CodegenMetadata& metadata,
absl::Duration* scheduling_time) {
return Schedule(p, metadata.scheduling_options, metadata.delay_estimator,
scheduling_time);
}

absl::StatusOr<CodegenResult> CodegenFromMetadata(
Package* p, GeneratorKind generator_kind, const CodegenMetadata& metadata,
const PipelineScheduleOrGroup* schedules, absl::Duration* codegen_time) {
if (generator_kind == GENERATOR_KIND_COMBINATIONAL) {
return CodegenCombinational(p, metadata.codegen_options,
metadata.delay_estimator, codegen_time);
}
XLS_RET_CHECK_EQ(generator_kind, GENERATOR_KIND_PIPELINE);
XLS_RET_CHECK(schedules != nullptr);
return CodegenPipeline(p, *schedules, metadata.codegen_options,
metadata.delay_estimator, codegen_time);
}

verilog::CodegenOptions::IOKind ToIOKind(IOKindProto p) {
switch (p) {
case IO_KIND_INVALID:
Expand Down Expand Up @@ -310,76 +414,58 @@ absl::StatusOr<CodegenResult> CodegenCombinational(
return CodegenResult{.module_generator_result = result};
}

absl::StatusOr<CodegenResult> ScheduleAndCodegen(
absl::StatusOr<PipelineScheduleOrGroup> Schedule(
Package* p,
const SchedulingOptionsFlagsProto& scheduling_options_flags_proto,
const CodegenFlagsProto& codegen_flags_proto, bool with_delay_model,
TimingReport* timing_report, PipelineScheduleOrGroup* schedules) {
if (!codegen_flags_proto.top().empty()) {
XLS_RETURN_IF_ERROR(p->SetTopByName(codegen_flags_proto.top()));
}
XLS_RET_CHECK(p->GetTop().has_value())
<< "Package " << p->name() << " needs a top function/proc.";

XLS_ASSIGN_OR_RETURN(verilog::CodegenOptions codegen_options,
CodegenOptionsFromProto(codegen_flags_proto));

if (codegen_flags_proto.generator() == GENERATOR_KIND_COMBINATIONAL) {
const DelayEstimator* delay_estimator = nullptr;
if (with_delay_model) {
XLS_ASSIGN_OR_RETURN(delay_estimator,
SetUpDelayEstimator(scheduling_options_flags_proto));
}
return CodegenCombinational(
p, codegen_options, delay_estimator,
timing_report ? &timing_report->codegen_time : nullptr);
}

// Note: this should already be validated by CodegenFlagsFromAbslFlags().
CHECK_EQ(codegen_flags_proto.generator(), GENERATOR_KIND_PIPELINE)
<< "Invalid generator kind: "
<< static_cast<int>(codegen_flags_proto.generator());

const CodegenFlagsProto& codegen_flags_proto,
absl::Duration* scheduling_time) {
XLS_RETURN_IF_ERROR(MaybeSetTop(p, codegen_flags_proto));
XLS_ASSIGN_OR_RETURN(
SchedulingOptions scheduling_options,
SetUpSchedulingOptions(scheduling_options_flags_proto, p));
CodegenMetadata metadata,
CodegenMetadata::Create(p, scheduling_options_flags_proto,
codegen_flags_proto, /*with_delay_model=*/true));
return ScheduleFromMetadata(p, metadata, scheduling_time);
}

QCHECK(scheduling_options.pipeline_stages() != 0 ||
scheduling_options.clock_period_ps() != 0)
<< "Must specify --pipeline_stages or --clock_period_ps (or both).";
absl::StatusOr<CodegenResult> Codegen(
Package* p,
const SchedulingOptionsFlagsProto& scheduling_options_flags_proto,
const CodegenFlagsProto& codegen_flags_proto, bool with_delay_model,
const PipelineScheduleOrGroup* schedules, absl::Duration* codegen_time) {
XLS_RETURN_IF_ERROR(MaybeSetTop(p, codegen_flags_proto));
XLS_ASSIGN_OR_RETURN(
CodegenMetadata metadata,
CodegenMetadata::Create(p, scheduling_options_flags_proto,
codegen_flags_proto, with_delay_model));
return CodegenFromMetadata(p, codegen_flags_proto.generator(), metadata,
schedules, codegen_time);
}

// Add IO constraints for RAMs.
for (const std::unique_ptr<xls::verilog::RamConfiguration>& ram_config :
codegen_options.ram_configurations()) {
for (const IOConstraint& ram_constraint : ram_config->GetIOConstraints()) {
scheduling_options.add_constraint(ram_constraint);
}
}
absl::StatusOr<CodegenResult> ScheduleAndCodegen(
Package* p,
const SchedulingOptionsFlagsProto& scheduling_options_flags_proto,
const CodegenFlagsProto& codegen_flags_proto, bool with_delay_model,
TimingReport* timing_report) {
XLS_RETURN_IF_ERROR(MaybeSetTop(p, codegen_flags_proto));
XLS_ASSIGN_OR_RETURN(
CodegenMetadata metadata,
CodegenMetadata::Create(p, scheduling_options_flags_proto,
codegen_flags_proto, with_delay_model));

XLS_ASSIGN_OR_RETURN(const DelayEstimator* base_estimator,
SetUpDelayEstimator(scheduling_options_flags_proto));
const FfiDelayEstimator ffi_estimator(
scheduling_options.ffi_fallback_delay_ps());
FirstMatchDelayEstimator delay_estimator("combined_estimator",
{base_estimator, &ffi_estimator});
XLS_RETURN_IF_ERROR(MaybeSetTop(p, codegen_flags_proto));

PipelineScheduleOrGroup temp_schedules = PackagePipelineSchedules();
if (schedules == nullptr) {
schedules = &temp_schedules;
}
XLS_ASSIGN_OR_RETURN(
*schedules,
Schedule(p, scheduling_options, &delay_estimator,
timing_report ? &timing_report->scheduling_time : nullptr));

if (p->GetTop().value()->IsProc()) {
// Force using non-pretty printed codegen when generating procs.
// TODO(tedhong): 2021-09-25 - Update pretty-printer to support
// blocks with flow control.
codegen_options.emit_as_pipeline(false);
PipelineScheduleOrGroup schedules = PackagePipelineSchedules();
PipelineScheduleOrGroup* schedules_ptr = nullptr;
if (codegen_flags_proto.generator() == GENERATOR_KIND_PIPELINE) {
XLS_ASSIGN_OR_RETURN(
schedules,
ScheduleFromMetadata(
p, metadata,
timing_report ? &timing_report->scheduling_time : nullptr));
schedules_ptr = &schedules;
}
return CodegenPipeline(
p, *schedules, codegen_options, &delay_estimator,
return CodegenFromMetadata(
p, codegen_flags_proto.generator(), metadata, schedules_ptr,
timing_report ? &timing_report->codegen_time : nullptr);
}

Expand Down
Loading

0 comments on commit f9f929b

Please sign in to comment.