Skip to content

Support start/end times during VCD reading for activity annotation#41

Open
stanminlee wants to merge 18 commits intomainfrom
vcd-timestamp
Open

Support start/end times during VCD reading for activity annotation#41
stanminlee wants to merge 18 commits intomainfrom
vcd-timestamp

Conversation

@stanminlee
Copy link

Supporting start and end times for reading VCDs, should populate activity/duty values for value changes within the time window specified.

Changes

  • Power.i + Power.tcl: TCL + SWIG interface for -start_time and -end_time arguments
  • VcdReader - Add filtering logic in incrCounts(). We do not count transitions outside of the time window. Added clipping for correct duty cycle in case the signal is high before the start_time.
  • VcdParse - avoid parsing vcd after end_time as a performance optimization + set time_min and time_max to filter boundaries so that the final calculations for activity and duty are correct.
  • Added test case in tests called vcd_timestamp

@greptile-apps
Copy link

greptile-apps bot commented Mar 16, 2026

Greptile Summary

This PR adds -start_time and -end_time arguments to the read_vcd command, allowing activity annotation to be restricted to a specific time window within a VCD file. The Tcl/SWIG layer cleanly forwards the new arguments, VcdParse stops parsing early once end_time is exceeded (performance optimisation), and VcdReader/VcdCount clip accumulated high-time and filter transition counts to the specified window.

Key findings:

  • Critical logic bug — time > end_time_ should be time >= end_time_ (power/VcdParse.cc:243): When a transition falls exactly on end_time, the parser does not break. The event is processed but VcdCount::incrCounts rejects it (time < filter_end is false). Because prev_value_ and prev_time_ are still updated by the rejected call, highTime(time_max) can no longer recover the correct contribution via its tail computation. For the test case -end_time 50, this produces duty=0.0 for u_inv/Y instead of the expected 1.0 — the regression test will fail as written.
  • Redundant end-boundary guard in VcdReader (VcdReader.cc:112): The time < filter_end half of count_interval becomes unreachable for the == boundary once the parser fix above is applied. Consider simplifying to just the start-time guard or adding a comment explaining the dual-filter intent.
  • setTimeMin called on every parseVarValues entry (VcdParse.cc:235): Since parseVarValues can be re-entered from the main token loop's else branch, the override fires multiple times per parse. Moving it to a single call-site would be cleaner.

Confidence Score: 2/5

  • Not safe to merge — the end_time boundary off-by-one will cause the new regression test to fail and produce incorrect duty cycle values for any window whose end aligns with a VCD transition.
  • The Tcl/SWIG/header changes are correct, and the start_time filtering logic works. However, the strict > comparison in VcdParse::parseVarValues causes the end_time boundary case to silently compute wrong duty cycles (0.0 instead of 1.0 in the test). The .ok expected output records the correct values, so the regression test itself will fail, confirming the bug is not yet exercised successfully by CI.
  • power/VcdParse.cc (line 243 — boundary bug), power/VcdReader.cc (line 112 — redundant end-boundary guard), test/vcd_timestamp.ok (will not match actual output until the fix lands)

Important Files Changed

Filename Overview
power/VcdParse.cc Adds start/end time filtering to the VCD parser. Contains a critical off-by-one bug: the break condition time > end_time_ should be time >= end_time_, which causes incorrect duty cycle output for the end-time boundary case (test 2 expects Y duty=1.0 but would compute 0.0). Also unconditionally calls setTimeMin on every entry to parseVarValues rather than once.
power/VcdReader.cc Adds filter_start_/filter_end_ fields to VcdCount, a clippedIntervalStart() helper, and a setTimeWindow method. The count_interval end-boundary condition (time < filter_end) becomes partially redundant (dead for == boundary) once the VcdParse break is fixed. The highTime() clipping logic is correct.
power/Power.tcl Extends read_vcd with -start_time and -end_time keys, defaulting both to -1 (sentinel for "unset"). Straightforward Tcl argument parsing with no issues.
power/Power.i SWIG interface updated to pass start_time and end_time as long long to the C++ layer. Clean, minimal change.
test/vcd_timestamp.tcl New regression test covering full read, end_time only, start_time only, and combined windowing. The typo report_activitie noted in a previous review has already been corrected here. The expected .ok output for the end_time=50 case will not match due to the VcdParse boundary bug.
test/vcd_timestamp.ok Expected output reflects the correct semantic (Y duty=1.0 for end_time=50), but the current implementation will produce 0.0, so this test will fail without the VcdParse >= fix.

Sequence Diagram

sequenceDiagram
    participant TCL as read_vcd (Power.tcl)
    participant SWIG as read_vcd_file (Power.i)
    participant RVA as ReadVcdActivities
    participant VCR as VcdCountReader
    participant VCP as VcdParse
    participant VCC as VcdCount

    TCL->>SWIG: read_vcd_file(filename, scope, start_time, end_time)
    SWIG->>RVA: ReadVcdActivities(filename, scope, start_time, end_time, sta)
    RVA->>VCR: setTimeWindow(start_time, end_time)
    RVA->>VCP: read(filename, &vcd_reader, start_time, end_time)

    Note over VCP: Stores start_time_, end_time_
    VCP->>VCR: setTimeMin(start_time_)  [if start_time >= 0]
    
    loop For each VCD token
        VCP->>VCP: parse timestamp #T
        alt T >= end_time (proposed fix: >=, current: >)
            VCP-->>VCP: break (stop parsing early)
        else T inside window
            VCP->>VCR: varAppendValue(id, T, value)
            VCR->>VCC: incrCounts(T, value, filter_start, filter_end)
            Note over VCC: count_interval = (T > filter_start) && (T < filter_end)<br/>accumulate high_time_ and transition_count_
        end
    end

    VCP->>VCR: setTimeMax(end_time_ if set, else time_)
    RVA->>VCR: timeMax() / timeMin() / countMap()
    RVA->>VCC: highTime(time_max) → duty cycle
    RVA->>RVA: setActivities() → power_.setUserActivity(pin, density, duty)
Loading

Last reviewed commit: c7f1711

@stanminlee
Copy link
Author

@greptile please review again

@stanminlee stanminlee changed the title Support start/end timesduring VCD reading for activity annotation Support start/end times during VCD reading for activity annotation Mar 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant