byuccl / bfasst

Tools for FPGA Assurance Flows
Apache License 2.0
12 stars 4 forks source link

VivadoSynthFromTcl tool is redundant #387

Closed KeenanRileyFaulkner closed 3 weeks ago

KeenanRileyFaulkner commented 7 months ago

The VivadoSynth tool located in bfasst/tools/synth/vivado_synth.py is sufficient for doing what the VivadoSynthFromTcl tool is doing. If you compare the following two files, you will see they are essentially doing the same thing when they call the vivado rule.

Generic vivado build snippet Redundant vivado build snippet

We could work to remove the VivadoSynthFromTcl class from the project (currently used only by the RandSoc flow), and the redundant template file, by simply placing a conditional mustach syntax around the build synth.tcl snippet in the file like this, and then updating the synth tool accordingly:

{{#create_tcl}}
build {{ synth_output }}/synth.tcl: template {{ synth_output }}/synth.json {{ synth_library }}/vivado_synth.tcl.mustache
{{/create_tcl}}

{{#in_context}}
build {{ synth_output }}/{{ top }}.xdc: vivado_ioparse {{ synth_output }}/report_io.txt
{{/in_context}}

build {{ synth_output }}/viv_synth.edf {{ synth_output }}/synth.dcp {{#in_context}}{{ synth_output }}/report_io.txt{{/in_context}}: vivado {{ synth_output }}/synth.tcl | {{#verilog}}{{ . }} {{/verilog}} {{#system_verilog}}{{ . }} {{/system_verilog}}
    journal = {{ synth_output }}/vivado.jou
    log = {{ synth_output }}/vivado.log
    cwd = {{ cwd }}

This would allow the synth tool to continue to operate as normal with regular synthesis, but proceed without creating the tcl file when the RandSoc tool does it instead. As it stands, having both tools is a bit confusing since Vivado Synth already uses a tcl file to operate.

jgoeders commented 7 months ago

Good points here. When I came up with the FromTcl naming, I was thinking that the design was originating from tcl vs HDL, and didn't think of the confusion with the fact that all the Vivado scripts use tcl.

We can try to merge these, but it might end up being easier to just merge just the template and not the Tool. The VivadoSynth tool is expecting a set of HDL files that it collects, which is quite different from the project-based Tcl that the other tool operates on.

yonnorc42 commented 2 months ago

@KeenanRileyFaulkner @jgoeders Do we want to implement this or should we close it?

reillymck commented 2 months ago

I have made this obsolete in my branch. I will work on getting that into main. @yonnorc42