Closed rowanG077 closed 2 months ago
One thing that seems to be missing here is clock delay/skew analysis. As it is inherently unlikely to have a hold time failure without clock skew (because in almost all cases, the clock to out time of a flipflop plus minimum routing is more than the hold time requirement of a DFF input), I think it would be good to do that at the same time.
However, it will require some consideration about performance, because this does add a bit of extra runtime to do the clock side analysis. Probably okay, but worth benchmarking and considering an option to disable it too.
That's true. I'm in the process of adding it.
The problem I'm facing is that global nets on ECP5 (which I use as my development and testing target since I know most about it) returns zero due to:
If I comment that ifdef I do get delays for the clock nets. Is this simply a case of ECP5 not having this information?
Yeah, unfortunately so, due to some questionable design decisions at the time they also aren't routed in a way that makes this easy to implement, either...
Is this the case for all architectures or only ECP5? If only on ECP5 I will simply switch to a different one and disable clock skew checking on ECP5.
Only ECP5, although as this is not a feature we've had in the past, I won't 100% guarantee they're actually accurate across all architectures. At a minimum they should be for iCE40, although its global networks are good enough and the device small enough that there's no skew (to test clock skew/hold analysis, you could always disable the use of them and then see a bunch of skew through the non-global routing).
Can I also add an option to disable global promotion for ECP5 like for ice40? I rather keep my test scripts the same. Even if not used in production it will allow me validate this PR. This will also close: https://github.com/YosysHQ/nextpnr/issues/1043
I think this is now in a reviewable state. I initially wanted store the clocks delay from all source FFs to sink FFs but this would become large and slow. I now moved the arrival and required times all in reference to the clock source. I also designed this PR to have a nice springboard to add min_delay
constraints since they are almost the same as the current hold check.
I did not yet benchmark. I will focus on that tomorrow. But I think the solution I choose shouldn't be very heavy. In the hot spots it's just an add/subtract more. What is more slowly now is the max_delay_by_domain_pairs
since it now needs to walk the critical path for non-related clocks. But since that is only used for reporting I'm fine with that.
Remaining TODOs:
I also changed the timing report to include the type of segment it is.
Info: Critical path report for clock 'clk$TRELLIS_IO_IN' (posedge -> posedge):
Info: type curr total
Info: clk-skew 52.00 52.00 Net clk$TRELLIS_IO_IN (15,14) -> (16,14)
Info: Sink out_TRELLIS_FF_Q_1.CLK
Info: Defined in:
Info: tests/simple.v:2.16-2.19
Info: clk-to-q 0.52 52.53 b_reg_TRELLIS_FF_Q_1.Q
Info: routing 0.00 52.53 Net b_reg[2] (15,14) -> (15,14)
Info: Sink b_reg_TRELLIS_FF_Q_1.Q
Info: Defined in:
Info: /nix/store/2bv02r255w3xgvcl00z1365zw6djv58f-yosys-0.38/bin/../share/yosys/ecp5/cells_map.v:108.23-108.24
Info: source 0.00 52.53 b_reg_TRELLIS_FF_Q_1.Q
Info: routing 1.10 53.62 Net b_reg[2] (15,14) -> (16,14)
Info: Sink out_TRELLIS_FF_Q_DI_PFUMX_Z_BLUT_LUT4_Z.B
Info: Defined in:
Info: /nix/store/2bv02r255w3xgvcl00z1365zw6djv58f-yosys-0.38/bin/../share/yosys/ecp5/cells_map.v:108.23-108.24
Info: logic 0.40 54.02 out_TRELLIS_FF_Q_DI_PFUMX_Z_BLUT_LUT4_Z.OFX
Info: routing 0.38 54.40 Net out_TRELLIS_FF_Q_DI[2] (16,14) -> (16,14)
Info: Sink out_TRELLIS_FF_Q_1.M
Info: Defined in:
Info: tests/simple.v:13.16-13.29
Info: /nix/store/2bv02r255w3xgvcl00z1365zw6djv58f-yosys-0.38/bin/../share/yosys/techmap.v:270.26-270.27
Info: setup 0.00 54.40 out_TRELLIS_FF_Q_1.M
Info: 0.93 ns logic, 53.47 ns routing
This is mainly done to make it more clear. Especially now with this PR there can be negative segments like the clk-skew
, clk-to-clk
and hold
segments.
I did 100 runs over the night with 18k lut design current master vs this branch:
Benchmark 1: make pnr FORCE_PNR=1 NEXTPNR_PATH="/home/rowan.goemans/Documents/engineering/nextpnr-orig/build/nextpnr-ecp5"
Time (mean ± σ): 60.214 s ± 4.609 s [User: 66.815 s, System: 0.310 s]
Range (min … max): 49.514 s … 82.134 s 100 runs
Benchmark 2: make pnr FORCE_PNR=1 NEXTPNR_PATH="/home/rowan.goemans/Documents/engineering/nextpnr/build/nextpnr-ecp5"
Time (mean ± σ): 60.108 s ± 4.370 s [User: 66.670 s, System: 0.305 s]
Range (min … max): 51.732 s … 76.546 s 100 runs
Doesn't seem to make an appreciable difference as expected. clock skew analysis was enabled during routing and final reporting. Since this doesn't have a real performance impact I would say just keep it on during routing and final reporting.
@gatecat Do you still want an explicit option to disable it? Due to the low performance impact and the extra safety I would just keep it always enabled. Perhaps even while doing placement.
Hmm I think I'm seeing some weird things. Do not press the merge button yet :).
Do you still want an explicit option to disable it? Due to the low performance impact and the extra safety I would just keep it always enabled. Perhaps even while doing placement.
Seems fine to keep it always enabled based on the metrics you've shown
I'm now confident this works correctly. The hold slack now matches the path delay reported which was not the case due to min/max bounds of a delay pair not being chosen correctly. So if you are happy with it. it can be merged.
Next up for me is min/max delay constraints. Since both can make use of existing infrastructure in the timing analyzer this shouldn't be too hard anymore.
Ex of hold time report:
Info: 1523 Hold/min time violations (showing 10 worst paths):
ERROR: Hold/min time violation for clock 'posedge clk50':
Info: type curr total name
Info: clk-skew -6.94 -6.94 Net clk50 (64,20) -> (65,20)
Info: Sink matrix_inst.TopEntity_topEntity_hubCore_result_18.c$app_arg_15_TRELLIS_FF_Q_2.CLK
Info: Defined in:
Info: verilog/TopEntity.topEntity/matrix.v:129.16-129.25
Info: clk-to-q 0.43 -6.51 Source matrix_inst.TopEntity_topEntity_hubCore_result_18.eta_0_TRELLIS_FF_Q_5.Q
Info: routing 0.49 -6.02 Net matrix_inst.TopEntity_topEntity_hubCore_result_18.eta_0[2] (64,20) -> (65,20)
Info: Sink matrix_inst.TopEntity_topEntity_hubCore_result_18.c$app_arg_15_TRELLIS_FF_Q_2.M
Info: Defined in:
Info: verilog/TopEntity.topEntity/TopEntity_topEntity_hubCore.v:249.14-249.19
Info: hold -0.30 -6.32 Source matrix_inst.TopEntity_topEntity_hubCore_result_18.c$app_arg_15_TRELLIS_FF_Q_2.M
Info: 0.13 ns logic, -6.45 ns routing
ERROR: Hold/min time violation for clock 'posedge clk50':
Info: type curr total name
Info: clk-skew -7.10 -7.10 Net clk50 (64,19) -> (64,23)
Info: Sink matrix_inst.TopEntity_topEntity_hubCore_result_18.a1_3_TRELLIS_FF_Q_63.CLK
Info: Defined in:
Info: verilog/TopEntity.topEntity/matrix.v:129.16-129.25
Info: clk-to-q 0.43 -6.67 Source matrix_inst.TopEntity_topEntity_hubCore_result_18.c$ds_app_arg_2_TRELLIS_FF_Q_63.Q
Info: routing 0.97 -5.70 Net matrix_inst.TopEntity_topEntity_hubCore_result_18.a1_4[33] (64,19) -> (64,23)
Info: Sink matrix_inst.TopEntity_topEntity_hubCore_result_18.a1_3_TRELLIS_FF_Q_63.M
Info: Defined in:
Info: verilog/TopEntity.topEntity/TopEntity_topEntity_hubCore.v:69.14-69.23
Info: hold -0.30 -6.00 Source matrix_inst.TopEntity_topEntity_hubCore_result_18.a1_3_TRELLIS_FF_Q_63.M
Info: 0.13 ns logic, -6.12 ns routing
ERROR: Hold/min time violation for clock 'posedge clk50':
Info: type curr total name
Info: clk-skew -7.07 -7.07 Net clk50 (64,20) -> (65,22)
Info: Sink matrix_inst.TopEntity_topEntity_hubCore_result_18.eta_0_TRELLIS_FF_Q_1.CLK
Info: Defined in:
Info: verilog/TopEntity.topEntity/matrix.v:129.16-129.25
Info: clk-to-q 0.43 -6.64 Source matrix_inst.TopEntity_topEntity_hubCore_result_18.flag_2_TRELLIS_FF_Q.Q
Info: routing 0.84 -5.80 Net matrix_inst.TopEntity_topEntity_hubCore_result_18.eta_0[0] (64,20) -> (65,22)
Info: Sink matrix_inst.TopEntity_topEntity_hubCore_result_18.bin'_2_LUT4_Z_2.B
Info: Defined in:
Info: verilog/TopEntity.topEntity/TopEntity_topEntity_hubCore.v:237.15-237.24
Info: logic 0.20 -5.61 Source matrix_inst.TopEntity_topEntity_hubCore_result_18.bin'_2_LUT4_Z_2.F
Info: routing 0.13 -5.47 Net matrix_inst.TopEntity_topEntity_hubCore_result_18.bin'_2[1] (65,22) -> (65,22)
Info: Sink matrix_inst.TopEntity_topEntity_hubCore_result_18.eta_0_TRELLIS_FF_Q_1.DI
Info: Defined in:
Info: verilog/TopEntity.topEntity/TopEntity_topEntity_hubCore.v:251.14-251.21
Info: hold -0.30 -5.78 Source matrix_inst.TopEntity_topEntity_hubCore_result_18.eta_0_TRELLIS_FF_Q_1.DI
Info: 0.33 ns logic, -6.11 ns routing
ERROR: Hold/min time violation for clock 'posedge clk50':
Info: type curr total name
Info: clk-skew -7.07 -7.07 Net clk50 (64,20) -> (65,22)
Info: Sink matrix_inst.TopEntity_topEntity_hubCore_result_18.eta_0_TRELLIS_FF_Q_2.CLK
Info: Defined in:
Info: verilog/TopEntity.topEntity/matrix.v:129.16-129.25
Info: clk-to-q 0.43 -6.64 Source matrix_inst.TopEntity_topEntity_hubCore_result_18.flag_2_TRELLIS_FF_Q.Q
Info: routing 0.84 -5.80 Net matrix_inst.TopEntity_topEntity_hubCore_result_18.eta_0[0] (64,20) -> (65,22)
Info: Sink matrix_inst.TopEntity_topEntity_hubCore_result_18.bin'_2_LUT4_Z_1.C
Info: Defined in:
Info: verilog/TopEntity.topEntity/TopEntity_topEntity_hubCore.v:237.15-237.24
Info: logic 0.20 -5.61 Source matrix_inst.TopEntity_topEntity_hubCore_result_18.bin'_2_LUT4_Z_1.F
Info: routing 0.13 -5.47 Net matrix_inst.TopEntity_topEntity_hubCore_result_18.bin'_2[0] (65,22) -> (65,22)
Info: Sink matrix_inst.TopEntity_topEntity_hubCore_result_18.eta_0_TRELLIS_FF_Q_2.DI
Info: Defined in:
Info: verilog/TopEntity.topEntity/TopEntity_topEntity_hubCore.v:251.14-251.21
Info: hold -0.30 -5.78 Source matrix_inst.TopEntity_topEntity_hubCore_result_18.eta_0_TRELLIS_FF_Q_2.DI
Info: 0.33 ns logic, -6.11 ns routing
ERROR: Hold/min time violation for clock 'posedge clk50':
Info: type curr total name
Info: clk-skew -6.42 -6.42 Net clk50 (65,19) -> (65,21)
Info: Sink matrix_inst.TopEntity_topEntity_hubCore_result_18.f1_1_TRELLIS_FF_Q_9.CLK
Info: Defined in:
Info: verilog/TopEntity.topEntity/matrix.v:129.16-129.25
Info: clk-to-q 0.43 -5.99 Source matrix_inst.TopEntity_topEntity_hubCore_result_18.a1_8_TRELLIS_FF_Q_9.Q
Info: routing 0.50 -5.49 Net matrix_inst.TopEntity_topEntity_hubCore_result_18.a1_7[23] (65,19) -> (65,21)
Info: Sink matrix_inst.TopEntity_topEntity_hubCore_result_18.c$ds_case_alt_selection_1_LUT4_Z_9.B
Info: Defined in:
Info: verilog/TopEntity.topEntity/TopEntity_topEntity_hubCore.v:104.15-104.24
Info: logic 0.20 -5.29 Source matrix_inst.TopEntity_topEntity_hubCore_result_18.c$ds_case_alt_selection_1_LUT4_Z_9.F
Info: routing 0.13 -5.16 Net matrix_inst.TopEntity_topEntity_hubCore_result_18.result_14[24] (65,21) -> (65,21)
Info: Sink matrix_inst.TopEntity_topEntity_hubCore_result_18.f1_1_TRELLIS_FF_Q_9.DI
Info: Defined in:
Info: verilog/TopEntity.topEntity/TopEntity_topEntity_hubCore.v:96.15-96.24
Info: hold -0.30 -5.46 Source matrix_inst.TopEntity_topEntity_hubCore_result_18.f1_1_TRELLIS_FF_Q_9.DI
Info: 0.33 ns logic, -5.79 ns routing
ERROR: Hold/min time violation for clock 'posedge clk50':
Info: type curr total name
Info: clk-skew -6.80 -6.80 Net clk50 (64,24) -> (64,22)
Info: Sink matrix_inst.TopEntity_topEntity_hubCore_result_18.TopEntity_topEntity_trueDualPortBlockRamWrapper_c$case_scrut_0.mem.0.5$DPRAM_COMB1.WCK
Info: Defined in:
Info: verilog/TopEntity.topEntity/matrix.v:129.16-129.25
Info: clk-to-q 0.43 -6.36 Source matrix_inst.TopEntity_topEntity_hubCore_result_18.result_46_TRELLIS_FF_Q_4.Q
Info: routing 1.12 -5.24 Net matrix_inst.result_19[62] (64,24) -> (64,22)
Info: Sink matrix_inst.TopEntity_topEntity_hubCore_result_18.TopEntity_topEntity_trueDualPortBlockRamWrapper_c$case_scrut_0.mem.0.5$RAMW_SLICE.A1
Info: Defined in:
Info: verilog/TopEntity.topEntity/matrix.v:131.16-131.27
Info: logic 0.00 -5.24 Source matrix_inst.TopEntity_topEntity_hubCore_result_18.TopEntity_topEntity_trueDualPortBlockRamWrapper_c$case_scrut_0.mem.0.5$RAMW_SLICE.WDO1
Info: routing 0.08 -5.16 Net matrix_inst.TopEntity_topEntity_hubCore_result_18.TopEntity_topEntity_trueDualPortBlockRamWrapper_c$case_scrut_0.mem.0.5$RAMW_SLICE$conn$WDO1 (64,22) -> (64,22)
Info: Sink matrix_inst.TopEntity_topEntity_hubCore_result_18.TopEntity_topEntity_trueDualPortBlockRamWrapper_c$case_scrut_0.mem.0.5$DPRAM_COMB1.WD
Info: hold -0.30 -5.46 Source matrix_inst.TopEntity_topEntity_hubCore_result_18.TopEntity_topEntity_trueDualPortBlockRamWrapper_c$case_scrut_0.mem.0.5$DPRAM_COMB1.WD
Info: 0.13 ns logic, -5.59 ns routing
ERROR: Hold/min time violation for clock 'posedge clk50':
Info: type curr total name
Info: clk-skew -6.52 -6.52 Net clk50 (62,19) -> (63,20)
Info: Sink matrix_inst.TopEntity_topEntity_hubCore_result_18.result_47_TRELLIS_FF_Q_1.CLK
Info: Defined in:
Info: verilog/TopEntity.topEntity/matrix.v:129.16-129.25
Info: clk-to-q 0.43 -6.09 Source matrix_inst.TopEntity_topEntity_hubCore_result_18.c$app_arg_15_TRELLIS_FF_Q_1.Q
Info: routing 0.94 -5.15 Net matrix_inst.TopEntity_topEntity_hubCore_result_18.c$app_arg_15[2] (62,19) -> (63,20)
Info: Sink matrix_inst.TopEntity_topEntity_hubCore_result_18.result_47_TRELLIS_FF_Q_1.M
Info: Defined in:
Info: verilog/TopEntity.topEntity/TopEntity_topEntity_hubCore.v:247.13-247.25
Info: hold -0.30 -5.45 Source matrix_inst.TopEntity_topEntity_hubCore_result_18.result_47_TRELLIS_FF_Q_1.M
Info: 0.13 ns logic, -5.58 ns routing
ERROR: Hold/min time violation for clock 'posedge clk50':
Info: type curr total name
Info: clk-skew -6.80 -6.80 Net clk50 (64,24) -> (64,21)
Info: Sink matrix_inst.TopEntity_topEntity_hubCore_result_18.TopEntity_topEntity_trueDualPortBlockRamWrapper_c$case_scrut_0.mem.0.6$DPRAM_COMB0.WCK
Info: Defined in:
Info: verilog/TopEntity.topEntity/matrix.v:129.16-129.25
Info: clk-to-q 0.43 -6.36 Source matrix_inst.TopEntity_topEntity_hubCore_result_18.result_46_TRELLIS_FF_Q_1.Q
Info: routing 1.18 -5.18 Net matrix_inst.result_19[65] (64,24) -> (64,21)
Info: Sink matrix_inst.TopEntity_topEntity_hubCore_result_18.TopEntity_topEntity_trueDualPortBlockRamWrapper_c$case_scrut_0.mem.0.6$RAMW_SLICE.C1
Info: Defined in:
Info: verilog/TopEntity.topEntity/matrix.v:131.16-131.27
Info: logic 0.00 -5.18 Source matrix_inst.TopEntity_topEntity_hubCore_result_18.TopEntity_topEntity_trueDualPortBlockRamWrapper_c$case_scrut_0.mem.0.6$RAMW_SLICE.WDO0
Info: routing 0.08 -5.11 Net matrix_inst.TopEntity_topEntity_hubCore_result_18.TopEntity_topEntity_trueDualPortBlockRamWrapper_c$case_scrut_0.mem.0.6$RAMW_SLICE$conn$WDO0 (64,21) -> (64,21)
Info: Sink matrix_inst.TopEntity_topEntity_hubCore_result_18.TopEntity_topEntity_trueDualPortBlockRamWrapper_c$case_scrut_0.mem.0.6$DPRAM_COMB0.WD
Info: hold -0.30 -5.40 Source matrix_inst.TopEntity_topEntity_hubCore_result_18.TopEntity_topEntity_trueDualPortBlockRamWrapper_c$case_scrut_0.mem.0.6$DPRAM_COMB0.WD
Info: 0.13 ns logic, -5.53 ns routing
ERROR: Hold/min time violation for clock 'posedge clk50':
Info: type curr total name
Info: clk-skew -7.14 -7.14 Net clk50 (64,20) -> (64,22)
Info: Sink matrix_inst.TopEntity_topEntity_hubCore_result_18.TopEntity_topEntity_trueDualPortBlockRamWrapper_c$case_scrut_0.mem.0.5$DPRAM_COMB3.WCK
Info: Defined in:
Info: verilog/TopEntity.topEntity/matrix.v:129.16-129.25
Info: clk-to-q 0.43 -6.71 Source matrix_inst.TopEntity_topEntity_hubCore_result_18.flag_2_TRELLIS_FF_Q.Q
Info: routing 0.69 -6.02 Net matrix_inst.TopEntity_topEntity_hubCore_result_18.eta_0[0] (64,20) -> (64,22)
Info: Sink matrix_inst.TopEntity_topEntity_hubCore_result_18.flag_2_LUT4_D.D
Info: Defined in:
Info: verilog/TopEntity.topEntity/TopEntity_topEntity_hubCore.v:237.15-237.24
Info: logic 0.20 -5.82 Source matrix_inst.TopEntity_topEntity_hubCore_result_18.flag_2_LUT4_D.F
Info: routing 0.47 -5.35 Net matrix_inst.TopEntity_topEntity_hubCore_result_18.TopEntity_topEntity_trueDualPortBlockRamWrapper_c$case_scrut_0.mem.0.0_WRE (64,22) -> (64,22)
Info: Sink matrix_inst.TopEntity_topEntity_hubCore_result_18.TopEntity_topEntity_trueDualPortBlockRamWrapper_c$case_scrut_0.mem.0.5$DPRAM_COMB3.WRE
Info: Defined in:
Info: verilog/TopEntity.topEntity/TopEntity_topEntity_trueDualPortBlockRamWrapper.v:42.3-50.6
Info: hold 0.00 -5.35 Source matrix_inst.TopEntity_topEntity_hubCore_result_18.TopEntity_topEntity_trueDualPortBlockRamWrapper_c$case_scrut_0.mem.0.5$DPRAM_COMB3.WRE
Info: 0.63 ns logic, -5.98 ns routing
ERROR: Hold/min time violation for clock 'posedge clk50':
Info: type curr total name
Info: clk-skew -7.14 -7.14 Net clk50 (64,20) -> (64,22)
Info: Sink matrix_inst.TopEntity_topEntity_hubCore_result_18.TopEntity_topEntity_trueDualPortBlockRamWrapper_c$case_scrut_0.mem.0.5$DPRAM_COMB0.WCK
Info: Defined in:
Info: verilog/TopEntity.topEntity/matrix.v:129.16-129.25
Info: clk-to-q 0.43 -6.71 Source matrix_inst.TopEntity_topEntity_hubCore_result_18.flag_2_TRELLIS_FF_Q.Q
Info: routing 0.69 -6.02 Net matrix_inst.TopEntity_topEntity_hubCore_result_18.eta_0[0] (64,20) -> (64,22)
Info: Sink matrix_inst.TopEntity_topEntity_hubCore_result_18.flag_2_LUT4_D.D
Info: Defined in:
Info: verilog/TopEntity.topEntity/TopEntity_topEntity_hubCore.v:237.15-237.24
Info: logic 0.20 -5.82 Source matrix_inst.TopEntity_topEntity_hubCore_result_18.flag_2_LUT4_D.F
Info: routing 0.47 -5.35 Net matrix_inst.TopEntity_topEntity_hubCore_result_18.TopEntity_topEntity_trueDualPortBlockRamWrapper_c$case_scrut_0.mem.0.0_WRE (64,22) -> (64,22)
Info: Sink matrix_inst.TopEntity_topEntity_hubCore_result_18.TopEntity_topEntity_trueDualPortBlockRamWrapper_c$case_scrut_0.mem.0.5$DPRAM_COMB0.WRE
Info: Defined in:
Info: verilog/TopEntity.topEntity/TopEntity_topEntity_trueDualPortBlockRamWrapper.v:42.3-50.6
Info: hold 0.00 -5.35 Source matrix_inst.TopEntity_topEntity_hubCore_result_18.TopEntity_topEntity_trueDualPortBlockRamWrapper_c$case_scrut_0.mem.0.5$DPRAM_COMB0.WRE
Info: 0.63 ns logic, -5.98 ns routing
Comments have been handled.
Based of https://github.com/YosysHQ/nextpnr/pull/1359