Skip to content

[ENG-1973] Bus timing#51

Merged
akashlevy merged 4 commits intomainfrom
bus-timing
May 2, 2026
Merged

[ENG-1973] Bus timing#51
akashlevy merged 4 commits intomainfrom
bus-timing

Conversation

@stanminlee
Copy link
Copy Markdown

Issue, given a bus and nested pin with the same timing types (when, related_pin, etc.) we will always pick the worst value (larger value), but we should pick the one that is nested deeper in the liberty cell's hierarchy.

Changes:

  • EDITS to finishPortGroups()
    • Populate bit_overrides_ data structure (maps ports to their timing groups) for future lookups (time saver)
    • Delete cell_port_groups after creating timing arcs to allow for override validation.
  • ADDED hasBitTimingOverride
    • Answers "does this bit in the bus have a pin declaration inside that should override the timing arc?"
    • If it doesn't we will create the timing arc using the BUS-level definitions
    • If it does, we will not create the timing arc and use the one created at the pin level
  • ADDED sameArcIdentity
    • Answers "now that we know there exists a pin declaration for this bus bit, does it match exactly to the bus timing group definitions?"
    • Essentially validates related_pin, timing_sense, etc.
  • ADDED relatedPinIncludesPort
    • Checks if the related pin between the bus and pin comparison we make are the same related pin.
  • EDITS to makeTimingArcs
    • We will not create a timing arc for bus bits that have such an override since we should use the nested declaration
    • Only applies to pin->bus and bus->bus paths since the destination bit is what is used for the timing arc (won't use a nested pin declaration for a bus->pin since the pin timing arcs are used)

@linear
Copy link
Copy Markdown

linear Bot commented May 2, 2026

ENG-1973

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 2, 2026

Greptile Summary

Introduces a two-pass strategy in finishPortGroups(): first populate a bit_overrides_ map (port → timing groups), then suppress bus-level timing arcs for bits that already have a matching nested pin() declaration, so that the more-specific pin-level arc wins instead of the previously observed "pick worst value" behaviour. The new sameArcIdentity / hasBitPinTimingOverride helpers check timing type, when condition, and related-pin identity before suppressing. All findings are P2.

Confidence Score: 4/5

Safe to merge with the two P2 gaps in mind; no data-loss or crash risk identified.

All findings are P2. The dangling-pointer situation is currently benign, and the missing guard in the no-related-pin overload is an edge case unlikely to appear in typical Liberty files. Core bus-override logic looks correct.

liberty/LibertyReader.cc — the no-related-pin makeTimingArcs overload and the bit_overrides_ lifetime after the deletion loop.

Important Files Changed

Filename Overview
liberty/LibertyReader.cc Adds relatedPinIncludesPort, sameArcIdentity, and hasBitPinTimingOverride helpers; refactors finishPortGroups to build bit_overrides_ map; guards bus-bit timing arc creation against pin-level overrides. The no-related_pin overload of makeTimingArcs is missing the new guard, and dangling pointers remain in bit_overrides_ after port-group deletion.
liberty/LibertyReaderPvt.hh Adds bit_overrides_ member and declarations for the three new helper functions; new declarations lack doc comments per project style.

Sequence Diagram

sequenceDiagram
    participant FPG as finishPortGroups()
    participant MTA as makeTimingArcs(PortGroup*)
    participant MTA2 as makeTimingArcs(from, to, timing)
    participant HBPTO as hasBitPinTimingOverride()
    participant SAI as sameArcIdentity()
    participant RPIP as relatedPinIncludesPort()
    participant B as builder_.makeTimingArcs()

    FPG->>FPG: Pass 1 — populate bit_overrides_
    FPG->>MTA: Pass 2 — makeTimingArcs(port_group)
    MTA->>MTA2: iterate bits of bus port
    MTA2->>HBPTO: hasBitPinTimingOverride(to_bit, from, timing)
    HBPTO->>SAI: sameArcIdentity(pin_t, bus_timing, from)
    SAI->>RPIP: relatedPinIncludesPort(pin_t, from, line)
    RPIP-->>SAI: true/false
    SAI-->>HBPTO: true/false
    HBPTO-->>MTA2: override exists?
    alt no override
        MTA2->>B: create timing arc
    else pin-level override exists
        MTA2-->>MTA2: skip (pin arc takes precedence)
    end
    FPG->>FPG: delete port groups
Loading

Comments Outside Diff (1)

  1. liberty/LibertyReader.cc, line 2694-2706 (link)

    P2 No-related_pin path skips the override check

    makeTimingArcs(LibertyPort *to_port, LibertyPort *related_out_port, TimingGroup *timing) is reached when a timing group has no related_pin attribute. It iterates over bus member bits and calls builder_.makeTimingArcs directly — without calling hasBitPinTimingOverride. If a bus(A) has a no-related-pin timing group and a nested pin(A[0]) declares the same no-related-pin timing group, the bus arc for A[0] is not suppressed and a duplicate timing arc is created. sameArcIdentity already handles the from_port == nullptr case, so the infrastructure is in place — the call just isn't wired up for this overload.

Reviews (1): Last reviewed commit: "remove comment" | Re-trigger Greptile

Comment thread liberty/LibertyReader.cc
Comment on lines +2057 to 2060
// Defer deletion until after the loop so overrides can safely walk.
for (PortGroup *port_group : cell_port_groups_)
delete port_group;
cell_port_groups_.clear();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Dangling pointers remain in bit_overrides_ after deletion loop

After the port-group deletion loop completes, bit_overrides_ still holds TimingGroup* pointers that now point to freed memory. The next call to finishPortGroups() clears the map before any use, so this is currently safe, but any subclass or future code that calls hasBitPinTimingOverride() between the end of finishPortGroups() and the next cell's finishPortGroups() would trigger undefined behaviour. A simple bit_overrides_.clear() immediately after the deletion loop makes the lifetime unambiguous.

Comment thread liberty/LibertyReaderPvt.hh
@akashlevy akashlevy merged commit 0a50ffc into main May 2, 2026
6 checks passed
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.

2 participants