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

[GSoC] Common Verilog Generation API and Implementation in temp-sense-gen #212

Merged
merged 32 commits into from
Jul 18, 2023

Conversation

harshkhandeparkar
Copy link
Collaborator

@harshkhandeparkar harshkhandeparkar commented May 31, 2023

Part of #211.

Changes

  • Added a common Python module in openfasoc/generators/common.
  • Added a common function for Verilog generation using Mako templates.
  • Added Mako as a dependency in requirements.txt.
  • Used the new Verilog generation function in the temp-sense-gen generator.
  • Converted temp-sense-gen Verilog source code to Mako templates.
  • Added tests for the new Python module in tests/ which are run by the pytest module in the CI (test_code.yml workflow)
  • Updated the documentation in docs/ to reflect the changes in filename and the new Mako-based templating format.

Results

  • The changed temp-sense-gen generator flow runs without any issues.

* uncommented code which inserts the designName in config.mk
* removed code which inserts the designName parameter in Verilog
* removed previous code used to generate output Verilog
* removed previous code used to copy generated Verilog
* removed code which replaces `.template` in source Verilog filenames
because it seemed unncessary
because it's output filename is the same as the input filename
* added docstring to the __init__.py file
* added docstrings to the functions defined in verilog_generation module
* added commonly used Verilog functions as defs in the Mako template
* updated the source verilog code to use the new defs
* added a TODO: comment to improve this way of importing defs in the future
@msaligane
Copy link
Member

@alibillalhammoud can you review too?

* uncommented code for generating `blocks/sky130hd/tempsenseInst_custom_net.txt` and `blocks/sky130hd/tempsenseInst_domain_insts.txt` files
* updated the source file path to reflect the changes in Verilog generation
Copy link
Collaborator

@alibillalhammoud alibillalhammoud left a comment

Choose a reason for hiding this comment

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

Looks good

* renamed all functions beginning with double _ to single _ to make them accessible using import <module_name>
@saicharan0112
Copy link
Collaborator

Apologies for the late response, I didn't knew that I was requested a review on this. Let me review this by today.

* Added unit tests using pytest in test/ directory
* Used part of temp-sense-gen Verilog for unit tests
* Added tests with 6 and 8 inverters
* tests if the directory structure of the source is maintained in the output
* tests if output for files in subdirectories are generated as expected
@msaligane
Copy link
Member

@saicharan0112 thanks! Any comments?

Copy link
Collaborator

@saicharan0112 saicharan0112 left a comment

Choose a reason for hiding this comment

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

The CI is passing and I see that temp-sense-gen CI is expecting files where it needs to be, so I can approve my changes based on these things.

@msaligane msaligane merged commit e421346 into idea-fasoc:main Jul 18, 2023
8 checks passed
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.

4 participants