Add stitched rtlsim liveness threshold override

[ollycassidy13]

Adds a builder config option to override stitched-IP rtlsim liveness thresholds.
This groups the builder config and verification-step environment handling for long MLO simulations.
Change type: Python

[STFleming]

Thanks @ollycassidy13!

This looks good, but I feel like maybe we are using it to avoid addressing an underlying issue .

Why was this developed, was it because the simulation was timing out before it had actually finished due to the derived threshold being too low? I'm concerned we're stacking band-aides and it might be better to address the underlying issue here.

Do you have a case where this was triggered that we can work from? The more minimal the better :)

[ollycassidy13]

Thanks @STFleming. This was not developed to allow a simulation that was falsely timing out to run for longer. The issue I encountered was the opposite: for a rolled MLO graph, the analytical estimate was too conservative, so a stall took millions of idle cycles before the watchdog fired.

The watchdog measures cycles without output activity, rather than total simulation duration. In the TinyDeiT case, the 12-iteration FINNLoop estimate was approximately 27.1M cycles, resulting in a derived watchdog of approximately 29.8M cycles. The corresponding RTL FIFO simulation completed in about 333k cycles, with an interval of about 295k cycles. This meant a real deadlock could spend roughly 30M simulated cycles before being reported.

The intended use of the override was therefore to select a smaller known-safe threshold - one still comfortably above the measured valid output interval - so genuine stalls fail earlier. It does not make a stalled simulation pass; it only changes when the existing watchdog reports it. The default remains unchanged when the option is unset.

The motivating case is the TinyDeiT FINNLoop, which is not especially minimal.

[STFleming]

Thanks so much for clarifying @ollycassidy13!

Is there any reason why the current LIVENESS_THRESHOLD env couldn't be used for this? (

finn/src/finn/util/basic.py

Line 150 in c3670d1

def get_liveness_threshold_cycles():

) I could see the argument for this being better suited to being a builder argument, but replacing the current LIVENESS_THRESHOLD envvar would break backwards compatibility so would require some thought.

[ollycassidy13]

The reason I added the builder argument was that, in the current STITCHED_IP_RTLSIM verification path, step_create_stitched_ip saves the existing LIVENESS_THRESHOLD, overwrites it with the derived critical-path value before calling verify_mlo/verify_step, and restores it afterward. Consequently, an explicitly supplied environment value does not control the watchdog during that verification step.

[STFleming]

The reason I added the builder argument was that, in the current STITCHED_IP_RTLSIM verification path, step_create_stitched_ip saves the existing LIVENESS_THRESHOLD, overwrites it with the derived critical-path value before calling verify_mlo/verify_step, and restores it afterward. Consequently, an explicitly supplied environment value does not control the watchdog during that verification step.

Ah interesting, thanks @ollycassidy13, is this intended @auphelia?

[merkelmarrow]

Thanks so much for clarifying @ollycassidy13!

Is there any reason why the current LIVENESS_THRESHOLD env couldn't be used for this? (

finn/src/finn/util/basic.py

Line 150 in c3670d1

def get_liveness_threshold_cycles():

) I could see the argument for this being better suited to being a builder argument, but replacing the current LIVENESS_THRESHOLD envvar would break backwards compatibility so would require some thought.

Just chiming in with what I found recently. LIVENESS_THRESHOLD reaches none of these:

• InsertAndSetFIFODepths computes max_iters = latency * 1.1 + 50 (set_fifo_depths.py) and passes it as timeout_cycles into rtlsim_exec_cppxsi
• step_measure_rtlsim_performance uses the same latency * 1.1 + 50 into xsi_fifosim()
• as noted above by @ollycassidy13, step_create_stitched_ip overwrites the env with the derived value first

[auphelia]

Thanks for looking into this and all your comments. I traced through the code and I think there's a root cause we should fix rather than adding a config workaround.

The underlying issue

The 29.8M vs 333k cycle discrepancy comes from FINNLoop.get_exp_cycles() in finn_loop.py:244-259:

body_cycles = loop_body.analysis(dataflow_performance)["critical_path_cycles"]
return (body_cycles + overhead_per_iter) * iteration

The problem is that critical_path_cycles sums all node latencies along the path (see dataflow_performance.py:73), which is explicitly noted as "very pessimistic" - it assumes no overlap between executions. For a pipelined dataflow this is wrong: nodes execute concurrently, so steady-state throughput is limited by max_cycles (the slowest node), not the sum.
Multiplying this pessimistic sum by iteration makes it dramatically worse.

On LIVENESS_THRESHOLD propagation

I see @ollycassidy13 and @merkelmarrow's points that LIVENESS_THRESHOLD isn't propagated throughout the codebase and we should address this.

Previously, @fpjentzsch suggested to always check LIVENESS_THRESHOLD and use it if it's higher than the estimate. At the time I was critical of that approach because it could mask estimation bugs. With bigger transformers now, I'm changing my opinion a bit.

I'd suggest:

1. Decrease the default LIVENESS_THRESHOLD (currently 1M cycles in get_liveness_threshold_cycles())
2. Allow LIVENESS_THRESHOLD to override the timeout only if it's set higher than the estimate. This way we don't mask bugs (estimate too high), but users can extend for legitimate edge cases (estimate too low)
3. Improve the timeout error message to include the derived estimate, so users know what "higher" means for their specific model. Something like:

RTL simulation timed out after {threshold} cycles (derived estimate: {estimate}).
If your model requires more cycles, set LIVENESS_THRESHOLD to a higher value.

This error message improvement should be added everywhere LIVENESS_THRESHOLD could be used.

Proposed fix

1. Fix FINNLoop.get_exp_cycles() to model pipelining correctly, something like:

  def get_exp_cycles(self):
      # ... existing annotation check ...

      iteration = self.get_nodeattr("iteration")
      perf = loop_body.analysis(dataflow_performance)

      # Pipeline: first output takes critical_path_cycles (fill),
      # subsequent outputs take max_cycles each (steady-state)
      body_latency = perf["critical_path_cycles"]
      body_throughput = perf["max_cycles"]
      overhead_per_iter = 40

      return body_latency + (iteration - 1) * body_throughput + overhead_per_iter * iteration

2. Decrease default LIVENESS_THRESHOLD and respect env var if higher than estimate
3. Give user a better error message to be able to see how they could set the LIVENESS_THRESHOLD
4. Remove stitched_rtlsim_liveness_threshold config option

Rationale

• Fix the root cause (bad estimate) rather than working around it
• Use the existing LIVENESS_THRESHOLD env var. It already exists, just needs to be respected consistently
• Override direction should be higher than the estimate only, preserving our ability to catch estimation bugs
• Better error messages help users self-serve when they hit legitimate edge cases

With a correct estimate, the TinyDeiT case should derive a reasonable threshold automatically.

[ollycassidy13]

Thanks, I’ve updated the PR with the proposed fix:

• FINNLoop.get_exp_cycles() now models pipeline fill with critical_path_cycles and steady-state iterations with max_cycles.
• Removed the builder-specific liveness override.
• Restored the default LIVENESS_THRESHOLD to 10k cycles and made it only increase derived estimates.
• Applied this behavior across stitched-IP, node-level, FIFO-sizing and performance simulations.
• Improved timeout errors to report the effective threshold and derived estimate.