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

Sky130 - Convert to Pydantic, support sky130_scl #848

Draft
wants to merge 68 commits into
base: master
Choose a base branch
from

Conversation

harrisonliew
Copy link
Contributor

@harrisonliew harrisonliew commented Mar 8, 2024

  • Implemented method gen_config for tech plugins to override and generate their own config (TechJSON instance, i.e. directly creating the Pydantic model). This enables multi-library support within the same tech plugin.
  • Removed all existing sky130-tech-gen-... scripts and outputs, static sky130.tech.json
  • Upstream @nayiri-k's tech LEF parsing of Metals into LEFUtils (unfortunately can't create Metal object directly or else it has a circular import)
  • Fixed gds_map_file reading in OpenROAD plugin

TODOs:

  • Add sky130_scl Cadence standard cell library option @elamdf
  • Documentation for this alternate method
  • Sky130 documentation for selecting library
  • e2e tests

Related PRs / Issues

Type of change:

  • Bug fix
  • New feature
  • Other enhancement

Impact:

  • Change to core Hammer
  • Change to a Hammer plugin
  • Other

Contributor Checklist:

  • Did you set master as the base branch?
  • Did you state the type-of-change/impact?
  • Did you delete any extraneous prints/debugging code?
  • (If applicable) Did you add documentation for the feature?
  • (If applicable) Did you update the poetry.lock file if you updated the requirements in pyproject.toml?
  • (If applicable) Did you add a unit test demonstrating the PR?
  • (If applicable) Did you run this through the e2e integration tests?
  • (If applicable) Did you update the submodules in e2e/ if this feature depends on updated plugins?

Copy link
Contributor

@nayiri-k nayiri-k left a comment

Choose a reason for hiding this comment

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

Passed through commercial/openroad e2e flow

@@ -53,4 +53,6 @@ def get_headers(source: str) -> List[str]:
# TODO: handle other compressed file types? Tools only seem to support gzip.
fd = os.open(source, os.O_RDONLY)
lines = os.pread(fd, nbytes, 0).splitlines()
return list(map(lambda l: l.decode('ascii', errors='ignore'), lines))
lines = list(map(lambda l: l.decode('ascii', errors='ignore'), lines))
all_lines = '\n'.join(lines).replace('\\\n','') # combine lines that are split by \ character
Copy link
Contributor

Choose a reason for hiding this comment

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

@harrisonliew This might be too hacky, but basically in one of the sky130 lib files (sky130_ef_io__gpiov2_pad_wrapped_ss_ss_100C_1v60_3v00.lib) the capacitive_load_unit declaration spanned 2 lines and it messed up the parsing in LIBUtils.get_cap_unit()

	capacitive_load_unit(1.000000, \
	  "pf");

Copy link
Contributor Author

@harrisonliew harrisonliew Mar 9, 2024

Choose a reason for hiding this comment

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

I think that's valid. But maybe the better approach is to remove line continuations (\) before processing lines. Can you also fix the type checker error?

@elamdf elamdf changed the title Sky130 - Convert to Pydantic, support sky130_scl Sky130 - Convert to Pydantic, support sky130_scl, e2e CI smoketest for sky130 Dec 23, 2024
@elamdf elamdf changed the title Sky130 - Convert to Pydantic, support sky130_scl, e2e CI smoketest for sky130 Sky130 - Convert to Pydantic, support sky130_scl, e2e CI smoketest Dec 23, 2024
- name: Checkout
uses: actions/[email protected]

- name: Run syn (Genus), par (Innovus), drc (Pegasus), and lvs (Pegasus) on the GCD design in sky130, using sky130_scl.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally the CI is fully reproducible. I don't know whats in the e2e folder or what poetry installs but in general I would make sure it's clear what tools are installed by default and what you are installing in the CI.

Copy link
Collaborator

@elamdf elamdf Dec 23, 2024

Choose a reason for hiding this comment

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

that's fair, we can remove the poetry call since this will only be run by one daemon on one machine, that is have it assume there's a poetry environment already set up- is that what you're asking? (It's unreasonable to have this run in isolation/install everything from scratch anyways since it relies on NFS paths for pdk collateral and tool binaries)

Comment on lines 29 to 37
echo "log file is $tempfile on $(hostname)" | tee -a $tempfile
echo "End-to-end CAD flow CI smoketest running on" ${{ github.head_ref }}.${{ github.sha }} > $tempfile 2>&1
make build >> $tempfile 2>&1
echo "running par.." | tee -a $tempfile
make par >> $tempfile 2>&1
echo "running drc.." | tee -a $tempfile
make drc >> $tempfile 2>&1
echo "running lvs.." | tee -a $tempfile
make lvs >> $tempfile 2>&1
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a huge fan of dumping something out to a file if it's sensitive. What happens if a current Hammer user accidentally removes one of these echos on accident and info is leaked? Maybe it's better to try to disable actions from logging entirely?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I see your point but I don't see how that reasoning doesn't apply to any output masking/secret hiding in an action. I've added a warning comment to hopefully dissuade accidental leaking, I'm not really sure how to actually resolve this concern- maybe only allow maintainers to manually kick off the workflow once they've checked if no malicious changes to the actions? That seems like a single point of failure though

@elamdf elamdf changed the title Sky130 - Convert to Pydantic, support sky130_scl, e2e CI smoketest Sky130 - Convert to Pydantic, support sky130_scl Feb 3, 2025
@elamdf
Copy link
Collaborator

elamdf commented Feb 3, 2025

will make a separate PR for CI smoketest

used_overrides.append((field_name, fname))
new_path = cached_paths[0]
self.logger.info(
f"overriding {default_path} with cache entry {new_path}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

this kind of blows up the log since there are a bunch of instances of the same library...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants