Skip to content

Reset the scheduler so that slack variables used to explain infeasibility do not corrupt future attempts to schedule. Also, always check if the user wants to try finding the minimum feasible clock period that would satisfy the scheduler while still propagating the scheduler's error #3598

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

Draft
copybara-service wants to merge 1 commit into
base: main
Choose a base branch
from
+143 −18
Draft

Reset the scheduler so that slack variables used to explain infeasibility do not corrupt future attempts to schedule. Also, always check if the user wants to try finding the minimum feasible clock period that would satisfy the scheduler while still propagating the scheduler's error #3598

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
2 changes: 2 additions & 0 deletions xls/scheduling/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -685,6 +685,7 @@ cc_test(
"@org_theopenroadproject//:opensta",
],
deps = [
":pipeline_schedule",
":pipeline_scheduling_pass",
":scheduling_options",
":scheduling_pass",
Expand All @@ -706,6 +707,7 @@ cc_test(
"//xls/passes:pass_base",
"@com_google_absl//absl/status:status_matchers",
"@com_google_absl//absl/status:statusor",
"@com_google_absl//absl/strings:str_format",
"@googletest//:gtest",
],
)
111 changes: 111 additions & 0 deletions xls/scheduling/pipeline_scheduling_pass_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "gtest/gtest.h"
#include "absl/status/status_matchers.h"
#include "absl/status/statusor.h"
#include "absl/strings/str_format.h"
#include "xls/common/file/get_runfile_path.h"
#include "xls/common/status/matchers.h"
#include "xls/common/status/status_macros.h"
Expand All @@ -37,6 +38,7 @@
#include "xls/ir/source_location.h"
#include "xls/ir/value.h"
#include "xls/passes/pass_base.h"
#include "xls/scheduling/pipeline_schedule.h"
#include "xls/scheduling/scheduling_options.h"
#include "xls/scheduling/scheduling_pass.h"

Expand Down Expand Up @@ -431,5 +433,114 @@ TEST_F(PipelineSchedulingPassTest, MultiProcScopedChannels) {
SchedulingOptions().pipeline_stages(2).schedule_all_procs(true)));
}

TEST_F(PipelineSchedulingPassTest, MinimizeClockOnFailure) {
auto p = CreatePackage();
FunctionBuilder fb("main", p.get());
BValue x = fb.Param("x", p->GetBitsType(32));
BValue y = fb.Param("y", p->GetBitsType(32));
BValue add = fb.Add(x, y);
BValue div = fb.UDiv(x, y);
BValue div_two = fb.UDiv(x, div);
BValue ored = fb.Or({add, div_two});
XLS_ASSERT_OK_AND_ASSIGN(Function * f, fb.BuildWithReturnValue(ored));

// Fail if trying to schedule with a clock period that is too small.
auto failed_scheduled_result = RunPipelineSchedulingPass(
f, SchedulingOptions().clock_period_ps(1).pipeline_stages(2));
EXPECT_THAT(failed_scheduled_result.status().message(),
AllOf(HasSubstr("cannot achieve the specified clock period"),
HasSubstr("clock_period_ps=3")));

// Succeed if trying to schedule with a large enough clock period.
XLS_ASSERT_OK_AND_ASSIGN(
auto success_scheduled_result,
RunPipelineSchedulingPass(
f, SchedulingOptions().clock_period_ps(3).pipeline_stages(2)));
EXPECT_THAT(success_scheduled_result,
Pair(true, SchedulingContextWithElements(UnorderedElementsAre(
Pair(f, VerifiedPipelineSchedule())))));
PipelineSchedule f_schedule =
success_scheduled_result.second.package_schedule().GetSchedule(f);
EXPECT_THAT(
f_schedule.GetCycleMap(),
UnorderedElementsAre(Pair(x.node(), 0), Pair(y.node(), 0),
Pair(add.node(), 0), Pair(div.node(), 0),
Pair(div_two.node(), 1), Pair(ored.node(), 1)));
}

TEST_F(PipelineSchedulingPassTest, MinimizeClockOnFailureWithIOConstraints) {
auto p = CreatePackage();
auto make_proc = [p = p.get()](
std::string_view name, Channel* channel_in,
Channel* channel_out) -> absl::StatusOr<Proc*> {
ProcBuilder pb(name, p);
BValue tok = pb.Literal(Value::Token());
BValue st = pb.StateElement("st", Value(UBits(0, 32)));
BValue recv = pb.Receive(channel_in, tok, SourceInfo(), "recv");
BValue recv_tok = pb.TupleIndex(recv, 0);
BValue recv_data = pb.TupleIndex(recv, 1);
for (int i = 0; i < 3; ++i) {
recv_data = pb.Add(recv_data, pb.Literal(UBits(1, 32)));
recv_data = pb.Subtract(recv_data, pb.Literal(UBits(1, 32)));
}
BValue add_st = pb.Add(st, recv_data);
pb.Send(channel_out, recv_tok, add_st, SourceInfo(), "send");
return pb.Build({add_st});
};

XLS_ASSERT_OK_AND_ASSIGN(
Channel * ch0, p->CreateStreamingChannel("ch0", ChannelOps::kReceiveOnly,
p->GetBitsType(32)));
XLS_ASSERT_OK_AND_ASSIGN(
Channel * ch1, p->CreateStreamingChannel("ch1", ChannelOps::kSendReceive,
p->GetBitsType(32)));
XLS_ASSERT_OK_AND_ASSIGN(
Channel * ch2, p->CreateStreamingChannel("ch2", ChannelOps::kSendOnly,
p->GetBitsType(32)));

XLS_ASSERT_OK(make_proc("proc0", ch0, ch1));
XLS_ASSERT_OK(make_proc("proc1", ch1, ch2));

auto failed_result = RunPipelineSchedulingPass(
p.get(), SchedulingOptions()
.clock_period_ps(2)
.pipeline_stages(4)
.add_constraint(IOConstraint(
/*source_channel=*/"ch0",
/*source_direction=*/IODirection::kReceive,
/*target_channel=*/"ch1",
/*target_direction=*/IODirection::kSend,
/*minimum_latency=*/1, /*maximum_latency=*/1))
.add_constraint(IOConstraint(
/*source_channel=*/"ch1",
/*source_direction=*/IODirection::kReceive,
/*target_channel=*/"ch2",
/*target_direction=*/IODirection::kSend,
/*minimum_latency=*/1, /*maximum_latency=*/1)));
EXPECT_THAT(
failed_result.status().message(),
AllOf(HasSubstr("cannot achieve the specified clock period"),
HasSubstr("clock_period_ps=4"),
HasSubstr("cannot satisfy the given I/O constraints. Would succeed "
"with: {ch0→ch1 with maximum latency ≥ 3}")));

XLS_ASSERT_OK(RunPipelineSchedulingPass(
p.get(), SchedulingOptions()
.clock_period_ps(4)
.pipeline_stages(4)
.add_constraint(IOConstraint(
/*source_channel=*/"ch0",
/*source_direction=*/IODirection::kReceive,
/*target_channel=*/"ch1",
/*target_direction=*/IODirection::kSend,
/*minimum_latency=*/1, /*maximum_latency=*/1))
.add_constraint(IOConstraint(
/*source_channel=*/"ch1",
/*source_direction=*/IODirection::kReceive,
/*target_channel=*/"ch2",
/*target_direction=*/IODirection::kSend,
/*minimum_latency=*/1, /*maximum_latency=*/1))));
}

} // namespace
} // namespace xls
48 changes: 30 additions & 18 deletions xls/scheduling/run_pipeline_schedule.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,12 +468,20 @@ absl::StatusOr<PipelineSchedule> RunPipelineScheduleInternal(
}

std::unique_ptr<SDCScheduler> sdc_scheduler;
// TODO: replace with handling reverting model constraint changes made while
// trying to explain infeasibility, thereby never needing to invalidate the
// SDCScheduler object regardless of settings.
bool invalidated = false;
auto initialize_sdc_scheduler = [&]() -> absl::Status {
if (sdc_scheduler == nullptr) {
XLS_ASSIGN_OR_RETURN(sdc_scheduler,
SDCScheduler::Create(f, io_delay_added));
XLS_RETURN_IF_ERROR(sdc_scheduler->AddConstraints(options.constraints()));
// We need to recreate the SDC scheduler because slack variables may be
// introduced if trying to explain infeasibility upon failing to schedule.
if (sdc_scheduler != nullptr && invalidated) {
sdc_scheduler.reset(nullptr);
invalidated = false;
}
XLS_ASSIGN_OR_RETURN(sdc_scheduler,
SDCScheduler::Create(f, io_delay_added));
XLS_RETURN_IF_ERROR(sdc_scheduler->AddConstraints(options.constraints()));
return absl::OkStatus();
};

Expand Down Expand Up @@ -620,12 +628,9 @@ absl::StatusOr<PipelineSchedule> RunPipelineScheduleInternal(
/*check_feasibility=*/false,
worst_case_throughput,
options.dynamic_throughput_objective_weight());
invalidated = !schedule_cycle_map.ok() &&
options.failure_behavior().explain_infeasibility;
if (!schedule_cycle_map.ok()) {
if (absl::IsInvalidArgument(schedule_cycle_map.status())) {
// The scheduler was able to explain the failure; report it up without
// further analysis.
return std::move(schedule_cycle_map).status();
}
if (options.clock_period_ps().has_value()) {
// The user specified a specific clock period; see if we can confirm
// that that's the issue.
Expand All @@ -649,8 +654,9 @@ absl::StatusOr<PipelineSchedule> RunPipelineScheduleInternal(
// Just increasing the clock period suffices.
return absl::InvalidArgumentError(absl::StrFormat(
"cannot achieve the specified clock period. Try "
"`--clock_period_ps=%d`.",
*min_clock_period_ps));
"`--clock_period_ps=%d`. The reason the scheduler gave is: "
"%v",
*min_clock_period_ps, schedule_cycle_map.status()));
}
if (absl::IsInvalidArgument(min_clock_period_ps.status())) {
// We failed with an explained error at the longest possible clock
Expand All @@ -659,8 +665,10 @@ absl::StatusOr<PipelineSchedule> RunPipelineScheduleInternal(
return xabsl::StatusBuilder(std::move(min_clock_period_ps).status())
.SetPrepend()
<< absl::StrFormat(
"cannot achieve the specified clock period; try "
"increasing `--clock_period_ps`. Also, ");
"cannot achieve the specified clock period; the "
"reason the scheduler gave is: %v. Try "
"increasing `--clock_period_ps`. Also, ",
schedule_cycle_map.status());
}
// We fail with an unexplained error even at the longest possible
// clock period. Report the original error.
Expand All @@ -686,9 +694,11 @@ absl::StatusOr<PipelineSchedule> RunPipelineScheduleInternal(
.status();
if (pessimistic_status.ok()) {
// Just increasing the clock period suffices.
return absl::InvalidArgumentError(
"cannot achieve the specified clock period. Try increasing "
"`--clock_period_ps`.");
return absl::InvalidArgumentError(absl::StrFormat(
"cannot achieve the specified clock period. The reason the "
"scheduler gave is: %v. Try increasing "
"`--clock_period_ps`.",
schedule_cycle_map.status()));
}
if (absl::IsInvalidArgument(pessimistic_status)) {
// We failed with an explained error at the pessimistic clock period.
Expand All @@ -697,8 +707,10 @@ absl::StatusOr<PipelineSchedule> RunPipelineScheduleInternal(
return xabsl::StatusBuilder(std::move(pessimistic_status))
.SetPrepend()
<< absl::StrFormat(
"cannot achieve the specified clock period; try "
"increasing `--clock_period_ps`. Also, ");
"cannot achieve the specified clock period; the reason "
"the scheduler gave is: %v. Try "
"increasing `--clock_period_ps`. Also, ",
schedule_cycle_map.status());
}
return pessimistic_status;
}
Expand Down
Loading