-
Notifications
You must be signed in to change notification settings - Fork 43
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
Add rule for writing GDS files #169
Conversation
Hi @QuantamHD and @proppy, please take a look at this PR |
gds_write/def2stream.py
Outdated
parser.add_argument("-o", "--out", required=True, help="Path to output GDS file") | ||
parser.add_argument("-g", "--additional-gds", required=False, help="Paths to additional input GDS files", default="", nargs="*") | ||
parser.add_argument("-f", "--fill-config", required=False, help="Path to JSON rule file for metal fill during chip finishing", default="") | ||
parser.add_argument("-s", "--seal", required=False, help="Path to seal GDS/OAS file", default="") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is seal
for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to OpenROAD-flow-scripts those seal GDS files are used in GF12 platform. At first I wanted to keep this part in case it would be needed in the future but I will remove it so that the script will be cleaner.
gds_write/def2stream.py
Outdated
print("[INFO] Checking for missing cell from GDS/OAS...") | ||
missing_cell = False | ||
regex = None | ||
if 'GDS_ALLOW_EMPTY' in os.environ: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we use a flag instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced with --gds-allow-empty
flag
load("@rules_hdl_pip_deps//:requirements.bzl", "requirement") | ||
load("@rules_python//python:defs.bzl", "py_binary") | ||
|
||
py_binary( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we have some py_test
too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we test the rule that uses this py_binary
. If that is not enough I can add py_test
but I think it would not be much different compared to the linked test. Please let me know what do you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll let @QuantamHD comment about the general approach taken by bazel_rules_hdl, but I'd think it'd be nice to have unittest for the python code as well.
place_and_route/build_defs.bzl
Outdated
@@ -60,6 +60,7 @@ def _place_and_route_impl(ctx): | |||
|
|||
return [ | |||
DefaultInfo(files = depset(output_files)), | |||
SynthesisInfo(top_module = ctx.attr.synthesized_rtl[SynthesisInfo].top_module), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious why this is required? (it looks unrelated)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be helpful if place_and_route
rule would return SynthesisInfo provider with the name of the top_module
from the synthesized design. This name is required in GDS write script, see e.g. here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @QuantamHD
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just return the ctx.attr.synthesized_rtl[SynthesisInfo] directly there's really no harm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
synthesis/tests/BUILD
Outdated
klayout_lyt = "klayout.lyt", | ||
klayout_lef = "klayout_tech.lef", | ||
fill_config = "fill.json", | ||
additional_lef = ["sky130_fd_sc_hd_merged.lef"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we get those thru dependencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid that if we place all those files under single dependencies
attribute we would have problems with distinguishing which file is which as they will be stored on a list and we would have to rely on file extensions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see. I was able to use PDK dependencies to pass down LEF and GDS files through StandardCellInfo
provider. From now on there is no need to specify input_lef
and input_gds
attributes. However I've left those attributes - they might be useful in some custom cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay,
Looks very good! I made a first round of comments with nits and questions about the implementation.
e127957
to
7d08218
Compare
Sorry for the long poll in review time. I'm committed to get that down to 24 hours. I will be more responsive going forward. Thank you for your contribution! You rock :) |
Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com>
c059e37
to
66cae28
Compare
Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com>
Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com>
Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com>
Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com>
Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com>
Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com>
Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com>
Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com>
Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com>
Check for possible orphans cells in new layout is redundant because we create new layout and explicitly copy there only the tree of the top cell so it is impossible to have there cells without parents other than the top cell. Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com>
Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com>
Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com>
Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com>
Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com>
Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com>
Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com>
Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com>
Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com>
Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com>
Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com>
Thanks! |
Closes #20, closes #54.
This PR adds
gds_write
rule which allows writing GDS files. The rule have a set of inputs and dependencies which are:place_and_route
target,Apart from the rules itself, PR:
pip_requirements.txt
place_and_route
rule to also returnSynthesisInfo
provider which is convenient for accessing top module name for the GDS write script.StandardCellInfo
provider to also store GDS files from PDKsdensity_fill
stage afterdetailed_routing