Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dbSta: Add create_cell_usage_snapshot command #6469

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bangqixu
Copy link

@bangqixu bangqixu commented Jan 4, 2025

Add create_cell_usage_snapshot command to create a cell usage snapshot in JSON format.

create_cell_usage_snapshot -path -stage <stage_name> will create a JSON file named as cell_usage_snapshot-<module_name>-<stage_name>.json under . The snapshot reflects (i) the name of the stage and (ii) cell usage statistics. Currently cell usage statistics consist of (a) cell name, (b) number of instance of the cell and (c) the cell area in um^2.

As compared to current implementation of report_cell_usage -v, this is more structured and more extensible and testable. We have planned work to support diff of such snapshots such that we can better evaluate the performance of each flow step / stage by capturing snapshots as often as needed.

@maliberty
Copy link
Member

We already have report_cell_usage so it would be better to enhance that than create a similar command.

@bangqixu
Copy link
Author

bangqixu commented Jan 4, 2025

Thanks for the feedback! I thought about that option and then realized that report_cell_usage at the moment is already a bit convoluted (the command itself reports a few different things and "-verbose" is not very expressive / self-explanatory wrt what is reported), so I think it is more readable and testable to create a separate command for this.

Besides, this is the first step of a series of create_.*_snapshot(s). We have plan to introduce timing histogram snapshot (and potentially a power snapshot as well) to capture the snapshot of PPA at a given moment. The general idea is to dump snapshots as JSON and separate utilities can then diff between snapshots to get the delta which will enable evaluation of flow steps.

IMHO, it would be cleaner to have create_.*_snapshots that creates snapshots as JSON files while the other commands can still report it as part of the tool (log) output.

If you have more constructive suggestions on how to achieve this without degrading readability and testability, I would be happy to help!

Let me know what you think.

cc @QuantamHD for viz

@maliberty
Copy link
Member

All reports are inherently snapshots so having both create_.snapshots and report. feels redundant and confusing to me, especially if they capture different information.

Having the option to generate a json report makes sense to me. I don't see it requiring a different command but just being an option to existing reporting.

If you are not fond of -verbose please explain what options you intend to create to replace it. We can always update the options for reporting to be more fine grained.

We also have the metrics format in json for dumping QOR information. Can what you need to represented as a metric?

@bangqixu
Copy link
Author

bangqixu commented Jan 5, 2025 via email

@maliberty
Copy link
Member

The metrics format is from https://vlsicad.ucsd.edu/Publications/Conferences/388/c388.pdf. You can run any design in ORFS to see the metrics reported by stage. It encodes information in the key name rather than introduce extra hierarchy in the json. I'd like to avoid having to support multiple formats unless there is a compelling reason.

What does -snapshot do? Is it just -json or -format json?

We already have utl::set_metrics_stage so -stage is duplicative.

Would -file make more sense than path? Are you writing more than one file?

@bangqixu
Copy link
Author

bangqixu commented Jan 5, 2025

Thanks for getting back to me!

I would argue that encoding information in the key name is not a great way to do this and explicitly having hierarchy in the data structure is a cleaner way. Plus if information is all encoded in the key, backward compatibility would be a pain as all entries need to be updated whereas if you have hierarchy and the extra attribute only exists on a low level, you can leave most of the information untouched. If I have to pick one, I would drop the support of flat / pure key-value pair one. It is similar spirit to that for liberty / LEF cell name, much information is encoded in the name (e.g., VT, cell height, etc), yet it is still more usable if such information is explicitly written down for easier query.

-snapshot is to indicate that the command is going to create a snapshot that matches the snapshot definition (again, with the same hierarchy between C++ code and JSON format for better readabilty / portability). If we do -json or -format json, that would not be less expressive what the command is going to output.

I would argue that -stage is more expressive as we don't need to introduce dependency on internal state set by utl::set_metrics_stage (plus this seems to be tied to the metrics concept).

I was thinking about creating snapshots for each module if there are hierarchy, so I put -path instead of -file there so that the format of the file name is coded and not arbitrary.

Thank you!

@maliberty
Copy link
Member

I would find it helpful if you wrote a document describing your full set of requirements rather than learning about them piecemeal in this discussion. I don't think a PR is the right place for this design discussion.

@bangqixu
Copy link
Author

bangqixu commented Jan 6, 2025

I will write a one-pager later this week.

@bangqixu
Copy link
Author

bangqixu commented Jan 9, 2025

Given the amount of time I can spend on this, I am going to change it to "report_cell_usage -format json -file".

@bangqixu bangqixu marked this pull request as draft January 9, 2025 08:14
@bangqixu
Copy link
Author

What would be the proper way to handle the module_inst argument of report_cell_usage? Currently it seems that the command assumes anything other than -verbose would be the name of the targeted module. Does it make sense to introduce -module key for the command (along with -file and -format). I feel like it would be cleaner to first introduce -module key (and update tests) before introducing -file and -format. Would breaking other's script a concern since this is effectively an API change?

@maliberty
Copy link
Member

You could add a -module option. You can also keep backward compatibility by treating any non-flag argument as the module name.

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