Add support for falling edge clocking #2211

lpawelcz · 2022-11-23T11:24:51Z

[This is a Work in Progress PR]

Description

This PR adds support for falling edge clocking.

Related Issue

Motivation and Context

All clocks and clock edge sensitivity of flip-flops / latches are implicitly assumed to be rising edge. Static timing analysis does not support falling edge clocking.

How Has This Been Tested?

I'm testing the flow with slightly modified examples given in documentation.

./vpr/vpr vtr_flow/arch/timing/EArch.xml vtr_flow/benchmarks/blif/multiclock/multiclock.blif --timing_report_detail debug --echo_file on --gen_post_synthesis_netlist on --gen_post_implementation_merged_netlist on

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

Signed-off-by: Pawel Czarnecki <[email protected]>

…falling edge clocked FFs as blif Signed-off-by: Pawel Czarnecki <[email protected]>

Signed-off-by: Pawel Czarnecki <[email protected]>

…ibrary Signed-off-by: Pawel Czarnecki <[email protected]>

Signed-off-by: Pawel Czarnecki <[email protected]>

Introduce secondary t_model and t_port to t_pb_types and secondary input/output/clock_pins in t_pb_graph_node. Also fill those fields with data required for processing. Signed-off-by: Pawel Czarnecki <[email protected]>

Signed-off-by: Pawel Czarnecki <[email protected]>

lpawelcz · 2022-12-21T16:03:26Z

Thank you for your comments! The code surely needs some fixes and cleanup. I'm working on that, as well as on adding required comments and documentation. I will go back to you as soon as possible after finishing this.

vaughnbetz

Finished the review (part 3 of 3)!
Many detailed comments but the big ones are:

Can we find an alternative to the inputs_sec / duplicate latch model? Not crazy about the data and code duplication it has led to, and the fact that it is harder to understand. Possible alternative: add an "is_inverted" property to input pins (or all pins) in the netlist. That would work for clocks and also be potentially useful for other purposes in the future. Currently we'd only set the is_inverted flag in one case: clock port on a falling edge latch, but could be used elsewhere in the future.

Not sure the tatum changes are all necessary; seems like we should be creating the appropriate 180 degree phase shift clock for any clock domain with neg_edge (inverted) registers, and then checking whether a register has the right edge polarity when putting it in the src tag and snk tag lists. I'm not sure the current code is doing that.

vaughnbetz · 2022-12-21T16:39:18Z

vpr/src/base/read_blif.cpp

+            arch_model = arch_models;
+            while (arch_model) {
+                if (strcmp(MODEL_LATCH, arch_model->name) == 0) {
+                    if (t_edge == arch_model->inputs[1].trigg_edge) {


Comment what inputs[1] is

Added a LATCH_CLOCK_INPUT_ID constant for this.

vaughnbetz · 2022-12-21T16:41:07Z

vpr/src/util/vpr_utils.h

@@ -109,6 +109,7 @@ int get_max_primitives_in_pb_type(t_pb_type* pb_type);
 int get_max_depth_of_pb_type(t_pb_type* pb_type);
 int get_max_nets_in_pb_type(const t_pb_type* pb_type);
 bool primitive_type_feasible(AtomBlockId blk_id, const t_pb_type* cur_pb_type);
+bool primitive_type_feasible(AtomBlockId blk_id, t_pb_graph_node* curr_pb_graph_node);


Comment what this does (I know lots of the other functions don't have comments, but we should improve that in new code)

Added comments

vaughnbetz · 2022-12-21T16:43:43Z

vpr/src/base/read_blif.cpp

@@ -466,6 +471,31 @@ struct BlifAllocCallback : public blifparse::Callback {
        return arch_model;
    }

+    const t_model* find_latch_model(TriggeringEdge t_edge) {


I don't think I saw a header file with this new function declared and a comment on what it does. If there isn't such a declaration it should be added, along with a comment.

It seams that this struct don't use a related header with function declarations. I added the comment here instead.

vaughnbetz · 2022-12-21T16:45:00Z

vpr/src/base/read_netlist.cpp


        for (int iport = 0; iport < gnode->num_input_ports; ++iport) {
            if (gnode->num_input_pins[iport] <= 0) continue;
+            pins = gnode->input_pins[iport];


Good use of a temporary variable to shorten the code below -- thanks!

vaughnbetz · 2022-12-21T16:46:44Z

vpr/src/pack/pb_type_graph.cpp

    for (i = 0; i < pb_type->num_ports; i++) {
-        if (pb_type->ports[i].model_port) {
+        if (pb_graph_node->has_secondary) {


Comment what this is doing. Also, I'm still concerned about duplicating all our ports and the latch primitive -- seems like there should be a way to control the clock edge that involves less data and code duplication.

Added comments

vaughnbetz · 2022-12-21T16:49:05Z

vpr/src/timing/read_sdc.cpp

-                    }
-                } else {
-                    auto net_aliases = netlist_.net_aliases(clock_name);
+        for (int i = 0; i < 2; i++) {


Suggest renaming i to iclk_edge (or something like that)
Comment what this is doing.

Renamed to clk_variant and added comments

vaughnbetz · 2022-12-21T17:33:28Z

vpr/src/util/vpr_utils.cpp

@@ -905,6 +905,91 @@ bool primitive_type_feasible(const AtomBlockId blk_id, const t_pb_type* cur_pb_t
    //Feasible
    return true;
 }
+bool primitive_type_feasible(const AtomBlockId blk_id, t_pb_graph_node* curr_pb_graph_node) {


Need a blank line above this (but maybe make format will put it in).

Added blank line

vaughnbetz · 2022-12-21T17:37:32Z

vpr/src/util/vpr_utils.cpp

@@ -936,20 +1021,32 @@ t_pb_graph_pin* get_pb_graph_node_pin_from_model_port_pin(const t_model_ports* m

    if (model_port->dir == IN_PORT) {
        if (model_port->is_clock == false) {
+            t_pb_graph_pin** input_pins;
+            if (model_port->trigg_edge == TriggeringEdge::FALLING_EDGE)


Again I'm not keen on this input_pins_sec etc. code, along with the duplicate latch. It's a lot of extra code and data to sort out if we have a positive or negative register; seems like there should be some additional data we can add to all primitives that would be simpler and cleaner.

Signed-off-by: Pawel Czarnecki <[email protected]>

…nput Signed-off-by: Pawel Czarnecki <[email protected]>

Signed-off-by: Pawel Czarnecki <[email protected]>

lpawelcz · 2022-12-23T14:03:36Z

For now I will leave the commits with all the fixes in case something needs to be reverted

This reverts commit 2353564.

This reverts commit ab19fee.

github-actions bot added external_libs libarchfpga Library for handling FPGA Architecture descriptions VPR VPR FPGA Placement & Routing Tool labels Nov 23, 2022

lpawelcz mentioned this pull request Nov 23, 2022

Support falling edge clocks (for FFs and timing analysis) #2182

Closed

lpawelcz force-pushed the pcza/negedge-clocking branch from 2c9b49e to 8727e59 Compare November 23, 2022 11:30

lpawelcz force-pushed the pcza/negedge-clocking branch 6 times, most recently from f346d81 to 083b4c8 Compare December 9, 2022 08:05

lpawelcz added 7 commits December 9, 2022 11:37

Libarchfpga: add 'edge' attribute to <port> under <model>

638a476

Signed-off-by: Pawel Czarnecki <[email protected]>

Tatum: store triggering edge info in TimingGraph

00c53c0

Signed-off-by: Pawel Czarnecki <[email protected]>

TimingGraph builder: write triggering edge info to TimingGraph

b9d570d

Signed-off-by: Pawel Czarnecki <[email protected]>

Tatum: show triggering clock edge in TimingGraph visualization

af81901

Signed-off-by: Pawel Czarnecki <[email protected]>

vpr: base: read_blif: add support for parsing falling edge clocked FFs

b4af78f

Signed-off-by: Pawel Czarnecki <[email protected]>

vpr: base: atom_netlist_utils: add support for printing netlist with …

c653664

…falling edge clocked FFs as blif Signed-off-by: Pawel Czarnecki <[email protected]>

Libarchfpga: add internal falling edge clocked .latch model

f1089f7

Signed-off-by: Pawel Czarnecki <[email protected]>

lpawelcz force-pushed the pcza/negedge-clocking branch 2 times, most recently from d42a69a to 76b9203 Compare December 9, 2022 11:36

lpawelcz added 10 commits December 9, 2022 12:36

vpr: test_interchange_device: expect second latch model in internal l…

4253fac

…ibrary Signed-off-by: Pawel Czarnecki <[email protected]>

vpr: utils: add second variant of primitive_type_feasible() check

560e5ee

Signed-off-by: Pawel Czarnecki <[email protected]>

vpr: pack: use second primitive_type_feasible() check

a82d8d4

Signed-off-by: Pawel Czarnecki <[email protected]>

libarchfpga: add secondary models to t_pb_type and t_pb_graph_node

a1f7c54

Introduce secondary t_model and t_port to t_pb_types and secondary input/output/clock_pins in t_pb_graph_node. Also fill those fields with data required for processing. Signed-off-by: Pawel Czarnecki <[email protected]>

libarchfpga: SyncModelsPbTypes: sync secondary models

725bc6b

Signed-off-by: Pawel Czarnecki <[email protected]>

vpr: utils: use primary or secondary pins

9033994

Signed-off-by: Pawel Czarnecki <[email protected]>

pack: pb_type_graph: use primary or secondary ports

c1004c1

Signed-off-by: Pawel Czarnecki <[email protected]>

base: read netlist: always assign primary pins

4df34e1

Signed-off-by: Pawel Czarnecki <[email protected]>

base: read netlist: fixup asserts

55c6c47

Signed-off-by: Pawel Czarnecki <[email protected]>

netlist writer: handle triggering clock edge in post-impl SDF writer

04cb06c

Signed-off-by: Pawel Czarnecki <[email protected]>

libtatum: CommonAnalysisVisitor: comment the tag propagation mechanism

2a4f193

Signed-off-by: Pawel Czarnecki <[email protected]>

vaughnbetz requested changes Dec 21, 2022

View reviewed changes

lpawelcz added 21 commits December 22, 2022 09:28

libarchfpga: arch_util: remove magic number for internal library size

079efe0

Signed-off-by: Pawel Czarnecki <[email protected]>

libarchfpga: arch_util: change all free() to delete

ab19fee

Signed-off-by: Pawel Czarnecki <[email protected]>

libarchfpga: change all .alloc() calls to 'new'

2353564

Signed-off-by: Pawel Czarnecki <[email protected]>

libarchfpga: comments for freeing internal models lib

e0a3b93

Signed-off-by: Pawel Czarnecki <[email protected]>

arch_util: cleanup comments

362821a

Signed-off-by: Pawel Czarnecki <[email protected]>

libarchfpga: add enum for internal models

0ce6545

Signed-off-by: Pawel Czarnecki <[email protected]>

libarchfpga: comments for module library latch duplication

5c76fcb

Signed-off-by: Pawel Czarnecki <[email protected]>

libarchfpga: physical_types: comment logical models

7f9c9b0

Signed-off-by: Pawel Czarnecki <[email protected]>

libarchfpga: physical_types: t_port: remove trigg_edge

d859ec3

Signed-off-by: Pawel Czarnecki <[email protected]>

libarchfpga: physical_types: comment ports

de3857d

Signed-off-by: Pawel Czarnecki <[email protected]>

libarchfpga: t_pb_type: remove num_ports_sec

977b9d6

Signed-off-by: Pawel Czarnecki <[email protected]>

libarchfpga: physical_types: comment update_pins

3e4cd85

Signed-off-by: Pawel Czarnecki <[email protected]>

vpr_utils: comment primitive_type_feasible

91369c9

Signed-off-by: Pawel Czarnecki <[email protected]>

libarchfpga: read xml: comments on processing secondary pb_type ports

ebfaea8

Signed-off-by: Pawel Czarnecki <[email protected]>

vpr: atom_netlist_utils: fixup latch handling in printing netlist

4649f0f

Signed-off-by: Pawel Czarnecki <[email protected]>

libarchfpga: models library: add constant for the ID of latch clock i…

cb46900

…nput Signed-off-by: Pawel Czarnecki <[email protected]>

vpr: pack: pb_type_graph: explain has_secondary usage

50fe438

Signed-off-by: Pawel Czarnecki <[email protected]>

vpr: timing: read_sdc: comment get_clocks

9996637

Signed-off-by: Pawel Czarnecki <[email protected]>

vpr_utils: explain TriggeringEdge usage in pb_graph_pin matching

f6e7fea

Signed-off-by: Pawel Czarnecki <[email protected]>

libarchfpga: physical_types: link to t_pb_types definition

e947026

Signed-off-by: Pawel Czarnecki <[email protected]>

formatting fixes

4ed6e5a

Signed-off-by: Pawel Czarnecki <[email protected]>

Revert "libarchfpga: change all .alloc() calls to 'new'"

aa0561b

This reverts commit 2353564.

lpawelcz force-pushed the pcza/negedge-clocking branch from 13f64eb to c59f504 Compare December 23, 2022 19:07

Revert "libarchfpga: arch_util: change all free() to delete"

c59f504

This reverts commit ab19fee.

lpawelcz changed the title ~~[WIP] Add support for falling edge clocking~~ Add support for falling edge clocking Dec 23, 2022

lpawelcz marked this pull request as ready for review December 23, 2022 20:39

Add support for falling edge clocking #2211

Are you sure you want to change the base?

Add support for falling edge clocking #2211

Uh oh!

Conversation

lpawelcz commented Nov 23, 2022

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

lpawelcz commented Dec 21, 2022

Uh oh!

vaughnbetz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpawelcz commented Dec 23, 2022

Uh oh!

Uh oh!