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

Templates for scripting #120

Merged
merged 18 commits into from
Jan 12, 2024
Merged

Templates for scripting #120

merged 18 commits into from
Jan 12, 2024

Conversation

micprog
Copy link
Member

@micprog micprog commented Mar 23, 2023

Modify script output to use custom templates (tera package). Allows use of custom script template files.

Fixes #124

@micprog micprog force-pushed the tpl_script branch 2 times, most recently from a27abdc to 8b7b8db Compare April 6, 2023 14:28
@micprog micprog marked this pull request as ready for review April 6, 2023 14:48
Copy link
Contributor

@bluewww bluewww left a comment

Choose a reason for hiding this comment

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

LGTM.

I tested flist (it also fixes #124) and vsim and vcs scripts output in carfield's safety island.

@micprog
Copy link
Member Author

micprog commented Apr 6, 2023

Review status for all script types:

  • flist (bluewww)
  • vsim (bluewww)
  • vcs (bluewww)
  • verilator
  • synopsys
  • formality
  • riviera
  • genus
  • vivado
  • vivado-sim
  • precision
  • custom template

Other checks (for all script types):

  • separate/common compilation mode
  • no-abort-on-error
  • VHDL sanity check

@micprog micprog requested a review from meggiman April 6, 2023 14:55
@jwnrt
Copy link
Contributor

jwnrt commented Apr 13, 2023

Apologies for what feels like a really obnoxious comment, but I was wondering what lead you to choose tera for the templating?

My motivation for asking is that it feels quite heavy with 68 transitive dependencies and adding ~50% to the build time. Maybe this isn't a metric you're measuring or optimizing for, which is totally fair!

From a quick survey of templating engines, tinytemplate is quite widely used and hopefully lightweight, but there are others too. I've only skimmed this PR's template usage, so maybe this doesn't have all the features you need, though.

This is a great feature regardless, thanks for all the work you're doing on it! I don't want to dismiss any of this work or suggest you redo anything!

@micprog
Copy link
Member Author

micprog commented Apr 13, 2023

Apologies for what feels like a really obnoxious comment, but I was wondering what lead you to choose tera for the templating?

My motivation for asking is that it feels quite heavy with 68 transitive dependencies and adding ~50% to the build time. Maybe this isn't a metric you're measuring or optimizing for, which is totally fair!

From a quick survey of templating engines, tinytemplate is quite widely used and hopefully lightweight, but there are others too. I've only skimmed this PR's template usage, so maybe this doesn't have all the features you need, though.

This is a great feature regardless, thanks for all the work you're doing on it! I don't want to dismiss any of this work or suggest you redo anything!

Hi @jwnrt,

The main reason I opted for tera was its similarity to other popular templating libraries, lowering the barrier of entry for those not familiar with Rust. The main motivation with this change is to make bender easier to use without having to figure out the intricacies within bender. It was also a good opportunity to streamline the script generation flow for all the different available tools.

I understand that the build time increases, but this is mainly a constraint on development - for deployment, there are easily available binary releases with a custom script for downloading. We have even recently added a custom github CI action to install bender within github actions. Build time of the tool is not a concern.

Regarding other template libraries, I have heard great things about mako, but this is limited to Python. In an effort to allow easier extension to other tools without needing to be familiar with Rust, I opted for a more advanced templating library than tinytemplate to allow more advanced features, in case they are necessary (I use replacements and conditions on loop state, which I don't think is possible with tinytemplate). Arguably, some things could be handled within the software, not the template, but the aim was to also provide the option for a user input template without needing to modify the Rust code. Also, not all libraries have equal active development.

Either way, thank you for your question, I hope this clarifies it somewhat. Let me know if you have feedback on the generated scripts with the new template!

@jwnrt
Copy link
Contributor

jwnrt commented Apr 14, 2023

Yep, that answers my question, thank you for the very detailed response!

src/cmd/script.rs Outdated Show resolved Hide resolved
@CyrilKoe
Copy link

CyrilKoe commented Jan 9, 2024

Tested vivado script, see Carfield pipeline.
Visual inspection of vivado-sim ok, only difference with before is that it now repeats certain defines:

set_property verilog_define [list \
    CLUSTER_ALIAS \
    CLUSTER_ALIAS \
    FEATURE_ICACHE_STAT \
    FEATURE_ICACHE_STAT \
    GEN_NO_HYPERBUS=1 \
    HIERARCHY_ICACHE_32BIT \
    HIERARCHY_ICACHE_32BIT \
    ICAHE_USE_FF \
    ICAHE_USE_FF \
    PRIVATE_ICACHE \
    PRIVATE_ICACHE \
    PULP_FPGA_EMUL \
    TARGET_BSCANE \

(Not the case before)

@fischeti
Copy link

fischeti commented Jan 9, 2024

Synopsys seems to work fine, I didn't see any issues pop during an elaboration step.

Add flist-plus script for files and plusargs
@CyrilKoe
Copy link

CyrilKoe commented Jan 9, 2024

Uniquify works for vivado/vivado-sim, now the script is the same as for previous bender version (just with with some re-ordering)

@fischeti
Copy link

fischeti commented Jan 9, 2024

scripts generated for verilator also seem to run through now

@micprog micprog merged commit 436377f into master Jan 12, 2024
5 checks passed
@micprog micprog deleted the tpl_script branch January 12, 2024 10:10
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.

Defines and Includes in flist
6 participants