f4pga-arch-defs icon indicating copy to clipboard operation
f4pga-arch-defs copied to clipboard

Move xc7 yosys script contents into reusable tcl functions

Open olofk opened this issue 3 years ago • 18 comments

Instead of calling python from the bash script wrapper, call it directly from conv.tcl

Signed-off-by: Olof Kindgren [email protected]

olofk avatar Jan 28 '22 12:01 olofk

Forgot to mention, this is kind of a first step in my objective to eventually let Edalize call the individual EDA tools directly instead of going through the shell script wrappers.

olofk avatar Jan 28 '22 12:01 olofk

Replaces https://github.com/SymbiFlow/symbiflow-arch-defs/pull/1461

olofk avatar Jan 28 '22 13:01 olofk

Ok, I redid the PR. Updating topic.

The new PR moves most of the code out of synth.tcl and conv.tcl into functions, so that these functions can be called by something else (e.g. Edalize) directly instead of going through symbiflow_synth. This means that the code is still backwards-compatible.

A few notes:

  1. The code from synth.tcl moved into new file synth_functions.tcl. Perhaps extending utils.tcl would make more sense?
  2. I tried to create functions in synth_functions.tcl that grouped together commands into what I believed was a full function. This might be very wrong though as I don't fully understand all the logic in synth.tcl. I could move stuff around or simply have one big function that does all of synth.tcl
  3. I reduced the number of env vars as much as possible and only made $SHARE_DIR_PATH and $TOP required. The other env vars were effectively hard coded anyway. I did keep USE_ROI and USE_LUT_CONSTANTS though as those felt more like global debug flags. But again, I'm not sure I got that right

olofk avatar Jan 30 '22 23:01 olofk

Updated now with restored TECHMAP_PATH and only calling python3 directly if there is no PYTHON3 env var

One thing. Do we need to register the new synth_functions.tcl in some CMake script or so to make sure it is installed? And do we want to move the synth_functions contents into utils.tcl instead or is it fine to have a new file for this?

olofk avatar Feb 02 '22 14:02 olofk

@olofk Good question, I missed this one actually. So IMO it might be ok to have all the synth functions alongside with the ones in the already present utils.tcl file, which is already getting installed along with the other scripts.

acomodi avatar Feb 02 '22 14:02 acomodi

I added all the functions to utils.tcl and updated the PR. It feels like they are similar to the ones already found there so I agree it make sense to have them all there

olofk avatar Feb 02 '22 14:02 olofk

I see that CI failed. Is this the issue? [TCL: yosys -import] Command name collision: found pre-existing command wbfliERROR: TCL interpreter returned an error: can't read "::env(TOP)": no such variable`

Think it might be. Force-pushing with this change

diff --git a/xc/xc7/yosys/synth.tcl b/xc/xc7/yosys/synth.tcl
index 55592d92..66ab227d 100644
--- a/xc/xc7/yosys/synth.tcl
+++ b/xc/xc7/yosys/synth.tcl
@@ -11,7 +11,7 @@ yosys -import
 
 source [file join [file normalize [info script]] .. utils.tcl]
 
-if { [info exists $::env(TOP)]} {
+if { [info exists ::env(TOP)]} {
     set top $::env(TOP)
 } else {
     set top ""

olofk avatar Feb 02 '22 21:02 olofk

I could also restore the env vars in synth.tcl instead. As long as we can have the functions in utils.tcl free from env vars that solves my original issue. That would make the diff smaller. Should I do it like that?

olofk avatar Feb 03 '22 11:02 olofk

Updated now to restore the env vars

olofk avatar Feb 03 '22 20:02 olofk

@olofk Can you please rebase on top of master, as there were some fixes to CI? I believe we can merge once everything goes green

acomodi avatar Feb 09 '22 20:02 acomodi

Done. Let's hope for the CI to say yes

olofk avatar Feb 09 '22 20:02 olofk

Stupid CI found another issue with my code. Pushed a new version now

olofk avatar Feb 10 '22 07:02 olofk

Oh, come on! Failed again? This time it must work

olofk avatar Feb 10 '22 09:02 olofk

Aarrrghh!!! Seems like there's some cmake scripts that needs updating. I have no idea what I'm doing anymore

olofk avatar Feb 10 '22 13:02 olofk

Ok, fuck it. I give up with the cmake stuff and simplified the PR instead so that it doesn't touch symbiflow_synth at all. This can't possibly go wrong

olofk avatar Feb 10 '22 20:02 olofk

Yes! All green. Good to go now @acomodi ?

olofk avatar Feb 11 '22 22:02 olofk

@olofk Great! All CIs now are green (I forgot to run kokoro as well which is still used to generate and push new device databases)

Given that there were some other merges in between, it would be good to rebase this PR on top of main and than merge

acomodi avatar Feb 22 '22 13:02 acomodi

@acomodi Rebased and repushed

olofk avatar Feb 22 '22 13:02 olofk