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

Smallfix to get liberty_arcs_one2one tests consistently passing in regression #86

Merged
merged 4 commits into from
Sep 17, 2024

Conversation

akashlevy
Copy link
Contributor

#85 highlighted that liberty_arcs_one2one fails under different machine configurations (via Docker). This PR fixes the issue by using the summary mode of report_checks to avoid having rising/falling edges be reported in a non-deterministic way.

@jjcherry56
Copy link
Collaborator

I think in this case you may want to use report_edges -from to query the graph directly instead of using reporting.

@akashlevy
Copy link
Contributor Author

True, I'm still relying on equi-delay paths being reported in a certain order. Let me go fix that...

@akashlevy
Copy link
Contributor Author

akashlevy commented Sep 13, 2024

Ok, I switched to using report_edges, but I'm seeing two potential issues...

  1. I'm confused why there seem to be four timing arcs per edge instead of two? An inverter is negative unate, so I don't know why we are seeing something that looks non-unate... does OpenSTA calculate unateness from the Liberty function?
  2. Is the order in which the different arcs are reported a deterministic thing for report_edges?

@jjcherry56
Copy link
Collaborator

The "function" is pretty ambiguous when you are talking about a bus. The bit port is a[0] but the function is on the bus port, not the bit port. It isn't smart enough to infer the timing sense so it defaults to non_unate. Just make it explicit like most libraries do.

I believe the reporting order should be deterministic. There was a big change to use object IDs
instead of pointers for better determinism, but it may not be perfect. Are you seeing platform
dependent results?

I want to see this broken into 2 different tests instead of reading 2 different verilog files.
I traditionally use a one line comment at the beginning of a regression so head -1 can show
what it is for. I only use file names to roughly group regressions and use numeric suffixes
to distinguish them.

@akashlevy
Copy link
Contributor Author

akashlevy commented Sep 13, 2024

Got it. I've added the timing sense to the Liberty files as suggested, which resolved the issue with non-unate arcs.

I have split and renamed the test cases. Added comments to line 1 of all tests describing what they do. Cross-platform regression runs are going through on my Mac as well as the two Linux Dockers.

Let me know if anything else is needed. Might be helpful to have a set of test guidelines in doc/CodingGuidelines.txt so contributors can follow that.

@jjcherry56 jjcherry56 merged commit 55069f5 into parallaxsw:master Sep 17, 2024
1 check passed
@akashlevy akashlevy deleted the libarcs_smallfix branch September 22, 2024 00:53
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