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

Expose dfflegalize -mince, -minsrst in synth scripts #3595

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

arjenroodselaar
Copy link

dfflegalize allows setting a threshold when it uses cells with CE ans SRST, but this is only exposed by synth_ice40. While troubleshooting YosysHQ/nextpnr#1062 I found that a combination of -mince and -minsrst would alleviate the placement pressure, allowing PnR to complete. This diff is a start to expose these knobs in every synth script.

Some test results for an iCE40 LP1K design, with no minimums (or -dff-min-ce 0 -dff-min-srst 0):

=== mkSidecarRevBTargetWithResetButton ===

   Number of wires:                895
   Number of wire bits:           3025
   Number of public wires:         895
   Number of public wire bits:    3025
   Number of memories:               0
   Number of memory bits:            0
   Number of processes:              0
   Number of cells:               1302
     SB_CARRY                       51
     SB_DFF                         93
     SB_DFFE                       229
     SB_DFFESR                      34
     SB_DFFESS                      34
     SB_DFFR                        24
     SB_DFFSR                       56
     SB_DFFSS                        1
     SB_IO                           6
     SB_LUT4                       774

With -dff-min-ce 4 -dff-min-srst 4:

=== mkSidecarRevBTargetWithResetButton ===

   Number of wires:                886
   Number of wire bits:           2982
   Number of public wires:         886
   Number of public wire bits:    2982
   Number of memories:               0
   Number of memory bits:            0
   Number of processes:              0
   Number of cells:               1302
     SB_CARRY                       51
     SB_DFF                         68
     SB_DFFE                       229
     SB_DFFESR                      67
     SB_DFFESS                      44
     SB_DFFR                        24
     SB_DFFSR                       39
     SB_IO                           6
     SB_LUT4                       774

Similar for a design targeting an ECP5-45F device, no minimums:

=== mkSidecarMainboardControllerRevB ===

  Number of wires:              32230
  Number of wire bits:         113931
  Number of public wires:       32230
  Number of public wire bits:  113931
  Number of memories:               0
  Number of memory bits:            0
  Number of processes:              0
  Number of cells:              44140
    $_TBUF_                         2
    CCU2C                         880
    DP16KD                          2
    L6MUX21                       478
    LUT4                        24894
    PFUMX                        3181
    TRELLIS_FF                  14703

And with -dff-min-ce 4 -dff-min-rsrst 8:

=== mkSidecarMainboardControllerRevB ===

  Number of wires:              32969
  Number of wire bits:         114685
  Number of public wires:       32969
  Number of public wire bits:  114685
  Number of memories:               0
  Number of memory bits:            0
  Number of processes:              0
  Number of cells:              45155
    $_TBUF_                         2
    CCU2C                         880
    DP16KD                          2
    L6MUX21                       614
    LUT4                        25258
    PFUMX                        3696
    TRELLIS_FF                  14703

I am more than happy to add this to the remaining synth scripts where applicable, but would like some review of the UI prior to doing so.

@nakengelhardt
Copy link
Member

Thanks for this PR! If they can make a noticeable difference in QoR for this target then it definitely makes sense to surface the options in the respective scripts.

Independently of that, it would also be nice to make these kind of QoR knobs accessible via the scratchpad, as we've started to do with various options in abc and abc9. That way, they can be changed even if a script pass doesn't have an option for it. I think all that would be needed in this case is to change the initialization in dfflegalize.cc:1024-1025 to this:

		mince = design->scratchpad_get_int("dfflegalize.mince", 0);
		minsrst = design->scratchpad_get_int("dfflegalize.minsrst", 0);

Then you should be able to use scratchpad -set dfflegalize.mince 4; scratchpad -set dfflegalize.minsrst 8; synth_ecp5 with the same result as synth_ecp5 -dff-min-ce 4 -dff-min-rsrst 8, but the scratchpad value would still be overridden if dfflibmap gets an explicit -mince or -minsrst argument. Could you try that out and add it to this PR?

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.

2 participants