The-OpenROAD-Project / OpenROAD

OpenROAD's unified application implementing an RTL-to-GDS Flow. Documentation at https://openroad.readthedocs.io/en/latest/
https://theopenroadproject.org/
BSD 3-Clause "New" or "Revised" License
1.41k stars 494 forks source link

gui: ensure inst pins are from register insts #5247

Closed AcKoucher closed 1 week ago

AcKoucher commented 1 week ago

The filter was not excluding instances that are not registers. So we might end displaying data of a path from/to e.g., a buffer.

This should have some impact on performance as the number of total endpoints will be generally smaller. E. g., ngt45/black-parrot:


Start Points
    Instance Pins 38501
    Register Pins 38477
End Points
    Instance Pins 111839
    Register Pins 76486
github-actions[bot] commented 1 week ago

clang-tidy review says "All clean, LGTM! :+1:"

maliberty commented 1 week ago

Do these non-reg end points go into some bucket? Do you have a sense of what sort of end points are being excluded in bp?

AcKoucher commented 1 week ago

@maliberty We're not distinguishing registers from other types of instances when using the filter. So currently the filter types are actually instance to instance, instance to IO, IO to instance.

As for the types of endpoints being excluded in bp:

StartPoints
    Instance Pins 38501
    Register Pins 38477
        Non-Register Instance Types
            Macro
EndPoints
    Instance Pins 111839
    Register Pins 76486
        Non-Register Instance Types
            Buffer/inverter
            Clock buffer/inverter
            Macro
            Multi-Input combinational cell
maliberty commented 1 week ago

I'm curious why so many non-sequential elements are endpoints. The SDC is quite simple so its not from that. What does one of these paths look like?

If we separate non-registers then we will have many more groups (eg reg2non-reg, input2non-reg, etc) which will be awkward.

AcKoucher commented 1 week ago

Well it looks like for the Macro endpoints we get paths:

As for the other non-register endpoints, it seems there are no paths, even though they're considered endpoints by sta.

maliberty commented 1 week ago

If there is no path then are they excluded from the histogram (I assume they have no slack either)?

maliberty commented 1 week ago

I don't feel we need to split registers and macros into separate categories. We could change reg to internal or non-IO but I don't have a strong opinion. @oharboe any preference?

oharboe commented 1 week ago

We want the definition of OpenSTA all_registers when we filter for "register to register".

all_registers include endpoint registers within macros.

AcKoucher commented 1 week ago

If there is no path then are they excluded from the histogram (I assume they have no slack either)?

Yes the buckets only contain Sequential Cells and Macros. I think this PR is not needed then.