Skip to content

Commit

Permalink
timing: Fix hold slack not matching reported path delay
Browse files Browse the repository at this point in the history
  • Loading branch information
@rowanG077 @gatecat
rowanG077 authored and gatecat committed Sep 24, 2024
1 parent 098dcae commit 93e233d
Show file tree
Hide file tree
Showing 4 changed files with 65 additions and 60 deletions.
8 changes: 3 additions & 5 deletions common/kernel/nextpnr_types.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -451,16 +451,14 @@ struct CriticalPath
// To cell.port
std::pair<IdString, IdString> to;
// Segment delay
DelayPair delay;
delay_t delay;
};

// Clock pair
ClockPair clock_pair;
// Total path delay
// if sum < 0 this is a hold/min violation
// if delay.maxDelay() >= max_delay this is a setup/max violation
DelayPair delay;

// if sum[segments.delay] < 0 this is a hold/min violation
// if sum[segments.delay] > max_delay this is a setup/max violation
delay_t max_delay;

// Individual path segments
Expand Down
15 changes: 6 additions & 9 deletions common/kernel/report.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,11 +73,11 @@ static Json::array json_report_critical_paths(const Context *ctx)
{"port", segment.to.second.c_str(ctx)},
{"loc", Json::array({toLoc.x, toLoc.y})}});

auto minDelay = ctx->getDelayNS(segment.delay.minDelay());
auto maxDelay = ctx->getDelayNS(segment.delay.maxDelay());

auto segmentJson =
Json::object({{"delay", Json::array({minDelay, maxDelay})}, {"from", fromJson}, {"to", toJson}});
auto segmentJson = Json::object({
{"delay", ctx->getDelayNS(segment.delay)},
{"from", fromJson},
{"to", toJson},
});

segmentJson["type"] = CriticalPath::Segment::type_to_str(segment.type);
if (segment.type == CriticalPath::Segment::Type::ROUTING) {
Expand Down Expand Up @@ -186,10 +186,7 @@ Report JSON structure:
},
"type": <path segment type "clk-to-q", "source", "logic", "routing" or "setup">,
"net": <net name (for routing only!)>,
"delay": [
<minimum segment delay [ns]>,
<maximum segment delay [ns]>,
],
"delay": <segment delay [ns]>,
}
...
]
Expand Down
47 changes: 33 additions & 14 deletions common/kernel/timing.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -895,8 +895,6 @@ CriticalPath TimingAnalyser::build_critical_path_report(domain_id_t domain_pair,
const auto &launch = domains.at(dp.key.launch).key;
const auto &capture = domains.at(dp.key.capture).key;

report.delay = DelayPair(0);

report.clock_pair.start.clock = launch.clock;
report.clock_pair.start.edge = launch.edge;
report.clock_pair.end.clock = capture.clock;
Expand Down Expand Up @@ -980,7 +978,7 @@ CriticalPath TimingAnalyser::build_critical_path_report(domain_id_t domain_pair,
if (!is_zero_delay(clock_delay)) {
CriticalPath::Segment seg_c2c;
seg_c2c.type = CriticalPath::Segment::Type::CLK_TO_CLK;
seg_c2c.delay = DelayPair(clock_delay);
seg_c2c.delay = clock_delay;
seg_c2c.from = std::make_pair(sp_cell->name, sp_clk_info.clock_port);
seg_c2c.to = std::make_pair(ep_cell->name, ep_clk_info.clock_port);
seg_c2c.net = IdString();
Expand All @@ -998,7 +996,7 @@ CriticalPath TimingAnalyser::build_critical_path_report(domain_id_t domain_pair,
if (!is_zero_delay(clock_skew)) {
CriticalPath::Segment seg_skew;
seg_skew.type = CriticalPath::Segment::Type::CLK_SKEW;
seg_skew.delay = DelayPair(clock_skew);
seg_skew.delay = clock_skew;
seg_skew.from = std::make_pair(sp_cell->name, sp_clk_info.clock_port);
seg_skew.to = std::make_pair(ep_cell->name, ep_clk_info.clock_port);
if (same_clock) {
Expand Down Expand Up @@ -1035,7 +1033,7 @@ CriticalPath TimingAnalyser::build_critical_path_report(domain_id_t domain_pair,
seg_logic.type = CriticalPath::Segment::Type::LOGIC;
}

seg_logic.delay = comb_delay.delayPair();
seg_logic.delay = longest_path ? comb_delay.maxDelay() : comb_delay.minDelay();
seg_logic.from = std::make_pair(prev_cell->name, prev_port);
seg_logic.to = std::make_pair(driver_cell->name, driver.port);
seg_logic.net = IdString();
Expand All @@ -1045,7 +1043,7 @@ CriticalPath TimingAnalyser::build_critical_path_report(domain_id_t domain_pair,

CriticalPath::Segment seg_route;
seg_route.type = CriticalPath::Segment::Type::ROUTING;
seg_route.delay = net_delay;
seg_route.delay = longest_path ? net_delay.maxDelay() : net_delay.minDelay();
seg_route.from = std::make_pair(driver_cell->name, driver.port);
seg_route.to = std::make_pair(sink_cell->name, sink.port);
seg_route.net = net->name;
Expand All @@ -1058,13 +1056,13 @@ CriticalPath TimingAnalyser::build_critical_path_report(domain_id_t domain_pair,

if (register_end) {
CriticalPath::Segment seg_logic;
seg_logic.delay = DelayPair(0);
seg_logic.delay = 0;
if (longest_path) {
seg_logic.type = CriticalPath::Segment::Type::SETUP;
seg_logic.delay += ep_clk_info.setup;
seg_logic.delay += ep_clk_info.setup.maxDelay();
} else {
seg_logic.type = CriticalPath::Segment::Type::HOLD;
seg_logic.delay -= ep_clk_info.hold;
seg_logic.delay -= ep_clk_info.hold.maxDelay();
}
seg_logic.from = std::make_pair(prev_cell->name, prev_port);
seg_logic.to = seg_logic.from;
Expand Down Expand Up @@ -1222,6 +1220,7 @@ std::vector<CriticalPath> TimingAnalyser::get_min_delay_violations()
for (auto &[launch_id, arr] : port.arrival) {
const auto &launch = domains.at(launch_id);
const auto &launch_clock = launch.key.clock;
const auto dom_pair_id = domain_pair_id(launch_id, capture_id);

auto clocks = std::make_pair(launch_clock, capture_clock);
auto related_clocks = clock_delays.count(clocks) > 0;
Expand All @@ -1236,17 +1235,37 @@ std::vector<CriticalPath> TimingAnalyser::get_min_delay_violations()
}

auto hold_slack = arr.value.minDelay() - req.value.maxDelay() + clock_to_clock;
auto violated = hold_slack <= 0;

if (violated) {
const auto dom_pair_id = domain_pair_id(launch_id, capture_id);
violations.emplace_back(build_critical_path_report(dom_pair_id, ep.first, false));
if (hold_slack <= 0) {
auto report = build_critical_path_report(dom_pair_id, ep.first, false);
violations.emplace_back(report);
}
}
}
}

return violations;
std::vector<std::pair<size_t, delay_t>> sum_indices;
sum_indices.reserve(violations.size());

for (size_t i = 0; i < violations.size(); ++i) {
delay_t delay = 0;
for (const auto &seg : violations[i].segments) {
delay += seg.delay;
}

sum_indices.emplace_back(i, delay);
}

std::sort(sum_indices.begin(), sum_indices.end(),
[](auto &left, auto &right) { return left.second < right.second; });

std::vector<CriticalPath> sorted_violations;
sorted_violations.reserve(violations.size());
for (const auto &pair : sum_indices) {
sorted_violations.push_back(std::move(violations[pair.first]));
}

return sorted_violations;
}

domain_id_t TimingAnalyser::domain_id(IdString cell, IdString clock_port, ClockEdge edge)
Expand Down
55 changes: 23 additions & 32 deletions common/kernel/timing_log.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,38 +69,29 @@ static void log_crit_paths(const Context *ctx, TimingResult &result)

// A helper function for reporting one critical path
auto print_path_report = [ctx](const CriticalPath &path) {
DelayPair total(0), logic_total(0), route_total(0);

// We print out the max delay since that's usually the interesting case
// But if we know this critical path has violated hold time we print the
// min delay instead
bool min_delay_violation = path.delay.minDelay() < 0;
auto get_delay_ns = [min_delay_violation, ctx](const DelayPair &d) {
if (min_delay_violation) {
ctx->getDelayNS(d.minDelay());
}
return ctx->getDelayNS(d.maxDelay());
};
delay_t total(0), logic_total(0), route_total(0);

log_info(" type curr total name\n");
for (const auto &segment : path.segments) {

total += segment.delay;
delay_t delay = segment.delay;

total += delay;

if (segment.type == CriticalPath::Segment::Type::CLK_TO_Q ||
segment.type == CriticalPath::Segment::Type::SOURCE ||
segment.type == CriticalPath::Segment::Type::LOGIC ||
segment.type == CriticalPath::Segment::Type::SETUP ||
segment.type == CriticalPath::Segment::Type::HOLD) {
logic_total += segment.delay;
logic_total += delay;

log_info("%10s % 5.2f % 5.2f Source %s.%s\n", CriticalPath::Segment::type_to_str(segment.type).c_str(),
get_delay_ns(segment.delay), get_delay_ns(total), segment.to.first.c_str(ctx),
ctx->getDelayNS(delay), ctx->getDelayNS(total), segment.to.first.c_str(ctx),
segment.to.second.c_str(ctx));
} else if (segment.type == CriticalPath::Segment::Type::ROUTING ||
segment.type == CriticalPath::Segment::Type::CLK_TO_CLK ||
segment.type == CriticalPath::Segment::Type::CLK_SKEW) {
route_total = route_total + segment.delay;
route_total = route_total + delay;

const auto &driver = ctx->cells.at(segment.from.first);
const auto &sink = ctx->cells.at(segment.to.first);
Expand All @@ -109,8 +100,8 @@ static void log_crit_paths(const Context *ctx, TimingResult &result)
auto sink_loc = ctx->getBelLocation(sink->bel);

log_info("%10s % 5.2f % 5.2f Net %s (%d,%d) -> (%d,%d)\n",
CriticalPath::Segment::type_to_str(segment.type).c_str(), get_delay_ns(segment.delay),
get_delay_ns(total), segment.net.c_str(ctx), driver_loc.x, driver_loc.y, sink_loc.x,
CriticalPath::Segment::type_to_str(segment.type).c_str(), ctx->getDelayNS(delay),
ctx->getDelayNS(total), segment.net.c_str(ctx), driver_loc.x, driver_loc.y, sink_loc.x,
sink_loc.y);
log_info(" Sink %s.%s\n", segment.to.first.c_str(ctx),
segment.to.second.c_str(ctx));
Expand Down Expand Up @@ -150,7 +141,7 @@ static void log_crit_paths(const Context *ctx, TimingResult &result)
}
}
}
log_info("%.2f ns logic, %.2f ns routing\n", get_delay_ns(logic_total), get_delay_ns(route_total));
log_info("%.2f ns logic, %.2f ns routing\n", ctx->getDelayNS(logic_total), ctx->getDelayNS(route_total));
};

// Single domain paths
Expand Down Expand Up @@ -180,16 +171,16 @@ static void log_crit_paths(const Context *ctx, TimingResult &result)
auto num_min_violations = result.min_delay_violations.size();
if (num_min_violations > 0) {
log_break();
log_info("Hold time/min time violations:\n");
log_info("%zu Hold/min time violations (showing 10 worst paths):\n", num_min_violations);
for (size_t i = 0; i < std::min((size_t)10, num_min_violations); ++i) {
auto &report = result.min_delay_violations.at(i);
log_break();
std::string start = clock_event_name(ctx, report.clock_pair.start);
std::string end = clock_event_name(ctx, report.clock_pair.end);
if (report.clock_pair.start == report.clock_pair.end) {
log_nonfatal_error("Hold time violation for clock '%s':\n", start.c_str());
log_nonfatal_error("Hold/min time violation for clock '%s':\n", start.c_str());
} else {
log_nonfatal_error("Hold time violation for path '%s' -> '%s':\n", start.c_str(), end.c_str());
log_nonfatal_error("Hold/min time violation for path '%s' -> '%s':\n", start.c_str(), end.c_str());
}
print_path_report(report);
}
Expand Down Expand Up @@ -230,13 +221,13 @@ static void log_fmax(Context *ctx, TimingResult &result, bool warn_on_failure)
log_break();

// Clock to clock delays for xpaths
dict<ClockPair, DelayPair> xclock_delays;
dict<ClockPair, delay_t> xclock_delays;
for (auto &report : result.xclock_paths) {
// Check if this path has a clock-2-clock delay
// clock-2-clock delays are always the first segment in the path
// But we walk the entire path anyway.
bool has_clock_to_clock = false;
DelayPair clock_delay = DelayPair(0);
delay_t clock_delay = 0;
for (const auto &seg : report.segments) {
if (seg.type == CriticalPath::Segment::Type::CLK_TO_CLK) {
has_clock_to_clock = true;
Expand Down Expand Up @@ -266,7 +257,7 @@ static void log_fmax(Context *ctx, TimingResult &result, bool warn_on_failure)
continue;
}

DelayPair path_delay(0);
delay_t path_delay = 0;
for (const auto &segment : report.segments) {
path_delay += segment.delay;
}
Expand All @@ -277,10 +268,10 @@ static void log_fmax(Context *ctx, TimingResult &result, bool warn_on_failure)
auto clock_delay = xclock_delays.at(report.clock_pair);

float fmax = std::numeric_limits<float>::infinity();
if (path_delay.maxDelay() < 0) {
fmax = 1e3f / ctx->getDelayNS(clock_delay.maxDelay());
} else if (path_delay.maxDelay() > 0) {
fmax = 1e3f / ctx->getDelayNS(path_delay.maxDelay());
if (path_delay < 0) {
fmax = 1e3f / ctx->getDelayNS(clock_delay);
} else if (path_delay > 0) {
fmax = 1e3f / ctx->getDelayNS(path_delay);
}

// Both clocks are related so they should have the same
Expand Down Expand Up @@ -322,7 +313,7 @@ static void log_fmax(Context *ctx, TimingResult &result, bool warn_on_failure)
auto ev_a = clock_event_name(ctx, pair.first.start, max_width_xca);
auto ev_b = clock_event_name(ctx, pair.first.end, max_width_xcb);

delay_t delay = pair.second.maxDelay();
delay_t delay = pair.second;
if (pair.first.start.edge != pair.first.end.edge) {
delay /= 2;
}
Expand All @@ -348,12 +339,12 @@ static void log_fmax(Context *ctx, TimingResult &result, bool warn_on_failure)
for (auto &report : result.xclock_paths) {
const ClockEvent &a = report.clock_pair.start;
const ClockEvent &b = report.clock_pair.end;
DelayPair path_delay(0);
delay_t path_delay = 0;
for (const auto &segment : report.segments) {
path_delay += segment.delay;
}
auto ev_a = clock_event_name(ctx, a, start_field_width), ev_b = clock_event_name(ctx, b, end_field_width);
log_info("Max delay %s -> %s: %0.02f ns\n", ev_a.c_str(), ev_b.c_str(), ctx->getDelayNS(path_delay.maxDelay()));
log_info("Max delay %s -> %s: %0.02f ns\n", ev_a.c_str(), ev_b.c_str(), ctx->getDelayNS(path_delay));
}
log_break();
}
Expand Down

0 comments on commit 93e233d

Please sign in to comment.