timing_builder: allow dangling combinational nodes

[acomodi]

Signed-off-by: Alessandro Comodi [email protected]

Description

In the SymbiFlow architectures, some of the tiles do not have combination sinks. This fix is needed to allow the architecture to have no combination sinks for specific tiles. Without this addition, the Timing graph builder would produce a tatum error and the failure of the VPR flow.

It may be better to have this fix under a command line option (e.g. --allow_non_combination_sinks).

Motivation and Context

Avoid Tatum error while building the Timing Graph within VPR.

Types of changes

• Bug fix (change which fixes an issue)
• New feature (change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My change requires a change to the documentation
• I have updated the documentation accordingly
• I have added tests to cover my changes
• All new and existing tests passed

[acomodi]

@kmurray, @vaughnbetz FYI

[kmurray]

@acomodi Can you give more context for why this is needed? Usually dangling combinational nodes means something is odd in the netlist, which is why it gets flagged by default.

I'm hesitant to always suppress it, since it often indicates real issues which would otherwise go undetected.

[acomodi]

@kmurray Sure, I will paste a full log of the error and additional debug files produced when dangling combinational nodes are not allowed.

Probably @litghost can provide further context on this issue.

[acomodi]

@kmurray if it helps here there are logs of a failing run when dangling combinational nodes are not allowed: logs.zip

[vaughnbetz]

@acomodi a dangling combinational node should indicate that the netlist has combinational logic that doesn't go anywhere, rather than a tile (physical block) that has no combinational sink.
I looked at the vpr_stdout.log file attached and it ends on these lines:
Build Timing Graph
Build Timing Graph took 0.00 seconds (max_rss 14.1 MiB, delta_rss +0.0 MiB)
Kevin, does dangling combinational logic print an error message (maybe it went to stderr and wasn't captured)?

[kmurray]

@vaughnbetz I believe the timing graph throws an exception during its consistency check, but the message doesn't appear in the log posted

@acomodi The following warnings look suspicious:

Warning 1: Model 'CE_VCC' output port 'VCC' has no timing specification (no clock specified to create a sequential output port, not combinationally connected to any inputs, not a clock output)
Warning 2: Model 'SR_GND' output port 'GND' has no timing specification (no clock specified to create a sequential output port, not combinationally connected to any inputs, not a clock output)
Warning 3: Model 'NO_FF' input port 'D' has no timing specification (no clock specified to create a sequential input port, not combinationally connected to any outputs, not a clock input)
Warning 4: Model 'NO_DRAM' input port 'A' has no timing specification (no clock specified to create a sequential input port, not combinationally connected to any outputs, not a clock input)
Warning 5: Model 'RAMB18E1_VPR' input port 'ADDRBTIEHIGH' has no timing specification (no clock specified to create a sequential input port, not combinationally connected to any outputs, not a clock input)
Warning 6: Model 'RAMB18E1_VPR' input port 'ADDRATIEHIGH' has no timing specification (no clock specified to create a sequential input port, not combinationally connected to any outputs, not a clock input)
Warning 7: Model 'RAMB36E1_PRIM' input port 'CASCADEINB' has no timing specification (no clock specified to create a sequential input port, not combinationally connected to any outputs, not a clock input)
Warning 8: Model 'RAMB36E1_PRIM' input port 'CASCADEINA' has no timing specification (no clock specified to create a sequential input port, not combinationally connected to any outputs, not a clock input)
Warning 9: Model 'RAMB36E1_PRIM' output port 'CASCADEOUTB' has no timing specification (no clock specified to create a sequential output port, not combinationally connected to any inputs, not a clock output)
Warning 10: Model 'RAMB36E1_PRIM' output port 'CASCADEOUTA' has no timing specification (no clock specified to create a sequential output port, not combinationally connected to any inputs, not a clock output)
Warning 11: Model 'VCC' output port 'VCC' has no timing specification (no clock specified to create a sequential output port, not combinationally connected to any inputs, not a clock output)
Warning 12: Model 'GND' output port 'GND' has no timing specification (no clock specified to create a sequential output port, not combinationally connected to any inputs, not a clock output)

They indicate that there are ports with no timing specifications which means the timing graph builder does not know what to do with them (no edges to add, not marked as sequential ports). As a result the timing graph consistency checks will fail.

I suspect fixing the architecture is a better approach than disabling the timing graph consistency checks universally, since they are aimed at catching these kinds of issues.

[vaughnbetz]

@kmurray Many of these definitely look suspicious and I agree they should be fixed.
Some look like vcc and gnd tie offs. What is your recommended way for specifying their timing? They don't really have a time dependency (they're stable from t = -infinity). A 0 delay edge is not too far off correct (a bit conservative) and a -ve delay edge is probably the most correct if it is allowed (that's what the old vpr classic timing analyzer did for constant generators).

[kmurray]

Some look like vcc and gnd tie offs. What is your recommended way for specifying their timing?

Historically we've only ever had 'regular' primitives act as constant generators -- that is blocks which otherwise have a 'well formed' timing model (edges/sources etc.) which can be inferred from the section of the architecture.

Whether any pins were constant (e.g. a block with no inputs) was handled as part of the timing constraints. VPR has existing code which detects constant generators from the netlist, and marks them as such in the timing constraints. Any timing nodes marked as constant generators are treated as sources launching data at t = -infinity.

For dedicated constant generators (i.e. tie-off primitives) we could extend the section to allow tagging the ports as such, and update the timing graph builder to just create SOURCE tnodes in those cases. That should ensure the timing graph produced passes all the legality checks within tatum. The existing constant generator inference code should handle tagging those pins as constants in the timing constraints.

However I wonder if it would be possible to model the tie-offs differently which would bypass this issue.
@acomodi Why are the dedicated tie-off primitives needed? Compared, for instance, to using regular constant-generator 0-LUTs.

[litghost]

@acomodi Why are the dedicated tie-off primitives needed? Compared, for instance, to using regular constant-generator 0-LUTs.

Because the fabric provides constant sources in many places in the graph (including within a site). For example, all LUT pins default to the VCC source by default (and consuming no routing resources). Every INT switch box has it's own dedicate GND source. Internal to sites are tieoff muxes, like the SR_GND and CE_VCC sources. In the case of the DSP block, there a special tieoff block dedicated for the DSP IPINs.

As for the constant-generator 0-LUTs, that is a pretty poor constant solutions, as that LUT gets routed across the entire fabric, consuming routing resources as it goes.

[kmurray]

After further reflection, I think the long term fix is to properly model tie-offs in the architecture (i.e. #163), and update the packer/router to be aware of them. That would remove the need to create special constant-generator primitives in the netlist as you do now (which is really just a work around for not having #163).

In the short term, making this a new command-line option (i.e. --allow_dangling_combinational_nodes, which defaults to off) likely makes sense. It would maintain the sanity checks by default but would allow the end-user to disable it if they have good reason to.

[litghost]

After further reflection, I think the long term fix is to properly model tie-offs in the architecture (i.e. #163), and update the packer/router to be aware of them. That would remove the need to create special constant-generator primitives in the netlist as you do now (which is really just a work around for not having #163).

In the XC7 architecture, the tie-offs generally speaking need to be routed. When I implemented the tie-offs as I did, I considered whether how a more specialize approach to #163 would go. In my head, I was unable to justify the complexity of adding explicit modelling support for tie-offs, versus just treating constant nets as nets.

Can you expand on how you would implement #163 that is different from adding blackbox's that are constant sources?

[kmurray]

Can you expand on how you would implement #163 that is different from adding blackbox's that are constant sources?

In the netlist I would have a 0-LUT for gnd and a 0-LUT for vcc. All primitive pins which are driven by constants would connect to one of those nets. This describes precisely and consistently the logical behaviour desired. Allowing stages which only care about the logical behaviour (e.g. timing graph) to be correct by construction.

With the tie-off modelling of #163 how that logical behaviour is actually implemented is architecture dependent optimization choice.

Pin tie-offs (which are really just additional inputs to the pin's input mux) would be modelled in the RR graph (e.g. intra-cluster RR graph) as additional SOURCE->IPIN connections. If a pin was tie-able to VCC/GND the pin would be connected to the VCC/GND SOURCE node. The standard router can then be invoked with only the VCC/GND source initialized.

The two implementation extremes are:

1. The architecture supports no tie-offs. This requires the outputs of the 0-LUTs to be routed just like regular signals to all constant primitive pins.
2. The architecture supports tie-offs at every pin. In this case it's always a single hop from the VCC/GND source to the pin being tied off.

Of course, most architecture would be somewhere in between with some tie-able pins and others which must have constants routed to them. This modelling approach handles the extreme and intermediate cases uniformly in a data-driven way.

[litghost]

In the netlist I would have a 0-LUT for gnd and a 0-LUT for vcc. All primitive pins which are driven by constants would connect to one of those nets. This describes precisely and consistently the logical behaviour desired. Allowing stages which only care about the logical behaviour (e.g. timing graph) to be correct by construction.

I agree that this is logically correct. However doing this requires VPR explicitly modelling that net as something other than a LUT.

Pin tie-offs (which are really just additional inputs to the pin's input mux) would be modelled in the RR graph (e.g. intra-cluster RR graph) as additional SOURCE->IPIN connections. If a pin was tie-able to VCC/GND the pin would be connected to the VCC/GND SOURCE node. The standard router can then be invoked with only the VCC/GND source initialized.

Almost none of the 7-series tie-offs look like this. There are 4 "https://github.com/verilog-to-routing/vtr-verilog-to-routing/issues/654types" of tie-offs seen:

• Routable tieoffs that a part of the local interconnect switch/connection box. Using this source requires first class routing. This cannot be approximated by an "IPIN" tieoff, because of how the switch box fans out the constant signal.
• Site internal tieoffs that are usually muxed with a routed signal internally to the site. These are not particular amendable to the IPIN tieoff model, because of the site routing mux. There is also additional complexity present here, because the packer doesn't have awareness (today) that a net is sourced from a constant source. Consider the following diagram taken from the SLICEM:
  

I was able to express this construct (this is the CE_VCC blackbox) using the existing architecture.

• Straight forward IPIN tieoffs (this are relatively rare)
• Implicit tieoff's (see How to express packing that introduces constant nets #654)

[acomodi]

@kmurray @litghost For the short term solution I have added the option to allow dangling combinational nodes.

[acomodi]

@kmurray @vaughnbetz ping

[acomodi]

@kmurray @vaughnbetz Ping

[acomodi]

@kmurray ping. I have rebased with the most recent updates on VTR

[kmurray]

Thanks!