Beginning of timing aware optimization using comb-depth and node count by danielpenas42 · Pull Request #228 · cornell-zhang/eqmap

danielpenas42 · 2026-04-27T03:04:34Z

No description provided.

matth2k · 2026-04-27T15:59:14Z

+    updated_region_mapping
+        .rewrite(&netlist)
+        .map_err(std::io::Error::other)?;
+


This time.rs file looks fine. But I think I want to take the time to untangle all the CLI flags for eqmap_asic and eqmap_fpga. I think using clap ValueEnum could cleanup most of eqmap_fpga, then we can just move the critical path partitioning code into the same file.

I will work on #226 eventually, then we can merge the code from here into eqmap_fpga.

matth2k · 2026-04-27T16:02:03Z

+/// Gets one critical path from the combinational-depth analysis.
+pub fn get_critical_path(
+    netlist: &Netlist<PrimitiveCell>,
+) -> Result<Vec<NetRef<PrimitiveCell>>, std::io::Error> {


This function is ultimately a wrapper around build_critical_path. So this function should return the same type: Option<Vec<NetRef>>

Maybe this function should just take the analysis as an argument. Then Option<Vec<NetRef>>> might be possible. Remember the ? operator works for Option types too.

matth2k · 2026-04-27T16:10:20Z

+/// A representative critical path ending at a timing endpoint.
+pub struct CriticalPath {
+    /// The output or register-input driver where this path ends.
+    pub endpoint: NetRef<PrimitiveCell>,


If this struct must be public, then the fields should probably be private. Have to keep some of the state encapsulated in this struct, or else people could set the depth, path, and endpoint to mismatched values.

For example, I don't think endpoint and depth are necessary separate fields. Is this what you want to do?

pub struct CriticalPath { /// The path through the critical fan-in path: Vec<NetRef<PrimitiveCell>>, } impl CriticalPath { pub fn new(...) { // Check path isn't empty and make new struct ... } pub fn depth(&self) -> usize { self.path.len() } pub fn endpoint(&self) -> NetRef<PrimitiveCell> { self.path[0].clone() } pub fn path(&self) -> &[NetRef<PrimitiveCell>] { // expose the vec as a slice } }

https://en.wikipedia.org/wiki/Encapsulation_(computer_programming)

matth2k · 2026-04-27T16:20:20Z

+) -> Result<HashSet<NetRef<PrimitiveCell>>, std::io::Error> {
+    let mut expanded_nodes: HashSet<NetRef<PrimitiveCell>> =
+        critical_path.iter().cloned().collect();
+    let mut frontier: Vec<NetRef<PrimitiveCell>> = critical_path.to_owned();


After the refactor, you should be able to do something like let frontier = critical_path.path().to_vec().

matth2k · 2026-04-27T16:20:45Z

+
+#[derive(Debug, Clone, PartialEq, Eq)]
+/// A representative critical path ending at a timing endpoint.
+pub struct CriticalPath {


Should this struct be renamed to DelayPath since it is not necessarily the most critical?

matth2k · 2026-04-27T16:21:36Z

+            for driver in node.drivers().flatten() {
+                if expanded_nodes.insert(driver.clone()) {
+                    next_frontier.push(driver);
+                }


matth2k · 2026-04-27T16:25:02Z

+}
+
+fn build_path_from_endpoint(
+    analysis: &CombDepthInfo<'_, PrimitiveCell>,


I like that this function takes the analysis directly as the argument, because then we know that the netlist is in a verified state.

Maybe the other functions should use the same pattern instead of building their own CombDepthInfo analysis. It will also speed up the code, because we will be running the timing pass once and reusing the result.

matth2k · 2026-04-27T16:31:27Z

+        current = crit
+            .get_driver()
+            .ok_or_else(|| std::io::Error::other("Circuit is ill-formed"))?
+            .unwrap();


I think this looks prettier as:

if let Some(c) = crit.get_driver() { current = c.unwrap(); } else { // Do the early exit thing return Err(...); }

Because currently we create a result type, just to call unwrap() on it. But honestly, it is a tie. Both aren't super pretty.

matth2k · 2026-04-27T16:36:52Z

 impl<'a, L: CircuitLang, I: Instantiable + LogicFunc<L>> LogicMapper<'a, L, I> {
    /// Map `nets` to [CircuitLang] nodes. `nets` that do not pass `filter_netref` *and* `filter_inst` become leaves.
-    fn insert_filtered<F, G>(
+    pub fn insert_filtered<F, G>(


If we make this method public, we should improve the docline for it. Like how filter_netref and filter_inst change how errors are thrown by this func. Something like the graph induced by filter_netref and filter_inst must be a DAG.

matth2k · 2026-04-27T16:39:20Z

+    let mut endpoints: Vec<(NetRef<PrimitiveCell>, usize)> = endpoints
+        .into_iter()
+        .filter_map(|endpoint| match analysis.get_comb_depth(&endpoint) {
+            Some(CombDepthResult::Depth(depth)) => Some((endpoint, depth)),


Do we need to separately retrieve the path depth? We know that re-building the path should give a vec that is the same length.

matth2k · 2026-04-27T16:40:06Z

+        .into_iter()
+        .take(n)
+        .map(|(endpoint, depth)| {
+            let path = build_path_from_endpoint(&analysis, endpoint.clone())?;


If a path fails to build, should we abort the function completely? Or just skip and try the next path? I think the latter is better.

matth2k · 2026-04-27T16:43:40Z

+        endpoints.insert(driver.unwrap());
+    }
+
+    for reg in netlist.matches(|inst| inst.is_seq()) {


Should we not be populating our endpoints with get_critical_points?

Should be able to do

// Take only the top-n most critical endpoints for node in analysis.get_critical_points().take(n) { ... }

Here is take() if you never used it before: https://doc.rust-lang.org/std/iter/struct.Take.html

matth2k · 2026-04-27T16:45:50Z

+
+/// Expands a critical path backward through fan-in for `n` frontier steps.
+pub fn expand_n_nodes(
+    critical_path: &[NetRef<PrimitiveCell>],


Think this function is fine. But shouldn't it take in as an input your critical path struct? Otherwise, you're not really using the abstraction you've built.

matth2k

Summary

Use analysis as a func argument, so we can reuse the verified and analyzed state of the netlist
Use your struct for encapsulation (no pub fields)
Try to avoid remapping None to errors unless we really need to. Just have the wrapping function return None
You should populate your endpoints using get_critical_points() not just any register.

matth2k · 2026-04-27T16:56:39Z

+        })
+        .collect::<Vec<_>>();
+
+    endpoints.sort_by(|(a_endpoint, a_depth), (b_endpoint, b_depth)| {


Once you refactor to use get_critical_points(), this sorting can be deleted. The iterator is designed to be sorted in order from most to least critical.

dp665 added 2 commits April 26, 2026 22:54

recent changes with some tests

2043868

fixes

b2fc8e5

matth2k self-requested a review April 27, 2026 15:57

matth2k reviewed Apr 27, 2026

View reviewed changes

matth2k requested changes Apr 27, 2026

View reviewed changes

matth2k reviewed Apr 27, 2026

View reviewed changes

Conversation

danielpenas42 commented Apr 27, 2026

Uh oh!

matth2k Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matth2k Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matth2k Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matth2k Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matth2k Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matth2k Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matth2k left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

matth2k Apr 27, 2026 •

edited

Loading

matth2k Apr 27, 2026 •

edited

Loading

matth2k Apr 27, 2026 •

edited

Loading

matth2k Apr 27, 2026 •

edited

Loading

matth2k Apr 27, 2026 •

edited

Loading

matth2k Apr 27, 2026 •

edited

Loading

matth2k left a comment •

edited

Loading