FPGA interchange: add RR graph generation #1999

acomodi · 2022-03-16T14:00:03Z

Description

This PR adds the RR graph generation, built from the RR interchange data.

The code present in the PR does mainly the following things:

reworks and reorganizes some code in common functions to avoid code duplication
adds switch to select the RR graph generation from interchange data
adds the code to generate the RR graph natively in VTR

Being the Interchange nodes and PIPs information different from the VTR RR graph data model, an intermediate step is required to read and preprocess the interchange data to be suitable for building the RR graph in VTR.

Motivation and Context

One of the missing steps in VTR to enable the Interchange format is to add the RR graph generation.

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: Maciej Dudek <[email protected]>

Code refactor Signed-off-by: Maciej Dudek <[email protected]>

Small improvements to RR graph generation Signed-off-by: Maciej Dudek <[email protected]>

Signed-off-by: Maciej Dudek <[email protected]>

…on step Combine node forest to single tree Signed-off-by: Maciej Dudek <[email protected]>

Update to new mainline Signed-off-by: Maciej Dudek <[email protected]>

Signed-off-by: Maciej Dudek <[email protected]>

…208f3 54f6208f3 Merge pull request SymbiFlow#69 from antmicro/mdudel/fix_annotations d8a0567a5 Fix annotations in DeviceResources git-subtree-dir: libs/EXTERNAL/libinterchange git-subtree-split: 54f6208f32e20b746863da6ea54e3051fc9a830e

Signed-off-by: Maciej Dudek <[email protected]>

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

Signed-off-by: Maciej Dudek <[email protected]>

Also fixes RR Graph generation with alternative site types Signed-off-by: Maciej Dudek <[email protected]>

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

acomodi · 2022-05-05T10:39:11Z

@vaughnbetz I have fixed formatting, as the PR was red, and everything is passing at the moment. I believe it is ready for an initial review

vaughnbetz · 2022-06-23T15:15:42Z

Adding @amin1377 and @tangxifan as this will be a big one for me to review on my own.

vaughnbetz · 2022-06-23T15:23:32Z

@amin1377 @tangxifan : context on this PR is in https://docs.google.com/document/d/1qc-iXMv3XzU21WV1qaQLtx7658nH_xqwIdqNOjR7qQM/edit#heading=h.iyijchq34gc2

tangxifan · 2022-06-24T02:21:19Z

@vaughnbetz No problem. Will do by the end of next week.

tangxifan

@acomodi Some more comments.

In the rr_graph build-up, I cannot find any codes to build the edges. If there is, can you point me to it? If no, should we address it in later PR?
I also do not find any code to validate the rr_graph once it is finished. Can you double check?

vtr-verilog-to-routing/libs/librrgraph/src/base/rr_graph_view.h

Lines 457 to 465 in b7a94b9

    
             public: /* Validators */ 
        
               /** brief Validate that edge data is partitioned correctly 
        
                * @note This function is used to validate the correctness of the routing resource graph in terms 
        
                * of graph attributes. Strongly recommend to call it when you finish the building a routing resource 
        
                * graph. If you need more advance checks, which are related to architecture features, you should 
        
                * consider to use the check_rr_graph() function or build your own check_rr_graph() function. */ 
        
               inline bool validate_node(RRNodeId node_id) const { 
        
                   return node_storage_.validate_node(node_id, rr_switch_inf_); 
        
               }

tangxifan · 2022-07-10T23:28:34Z

libs/libarchfpga/src/read_fpga_interchange_arch.cpp


-        auto timing_data = ar_.getPipTimings();
+        arch_->num_switches = seen.size() + 2;


Would you mind to add comments to explain the magic number 2? It looks to me about the hidden switches added by VPR. But not sure about it.

This +2 is to store MUX and SHORT switch types. arch_->Switches is filled out in process_switches_array function, where SHORT is give id 0 and MUX is given id 1, and rest is used for architecture switches.

I'll add comment in code explaining this.

Thanks. That will help developers a lot!

tangxifan · 2022-07-10T23:32:00Z

libs/libarchfpga/src/read_fpga_interchange_arch.cpp

@@ -2295,10 +2235,10 @@ struct ArchReader {
                grid_def.loc_defs.emplace_back(std::move(single));
            }

-            // The constant source tile will be placed at (0, 0)
+            // The constant source tile will be placed at (1, max_height)


It makes sense to have a fixed layout when using FPGA interchange file format.
Should we comment more about the regulation? Or find a way to document this?

In line 2194 fixed grid size is forced, so there is no option to select auto size for FPGA,
but there is no comment about it. Maybe I could add information about this restriction in VPR help for fpga_interchange option? What is your opinion about this?

Yes. Because this is a restriction, we should have necessary code comments to alert developers.

tangxifan · 2022-07-10T23:49:33Z

libs/libarchfpga/src/read_fpga_interchange_arch.cpp

@@ -258,6 +168,29 @@ static t_pin_to_pin_annotation get_pack_pattern(std::string pp_name, std::string
    return pp;
 }

+static void add_segment_with_default_values(t_segment_inf& seg, std::string name) {


I guess the default values are consistent with this function

vtr-verilog-to-routing/libs/libarchfpga/src/read_xml_arch_file.cpp

Lines 3524 to 3530 in b7a94b9

static void ProcessSegments(pugi::xml_node Parent,

std::vector<t_segment_inf>& Segs,

const t_arch_switch_inf* Switches,

const int NumSwitches,

const bool timing_enabled,

const bool switchblocklist_required,

const pugiutil::loc_data& loc_data) {

Should we consider to deposit these value when creating the object?

I've looked at this function, and fpga_interchange defaults are different to ones set in xml reader.
So I'm not positive moving default value into object constructor is good idea.

O.K. Not a problem to me.

tangxifan · 2022-07-10T23:53:29Z

libs/libarchfpga/src/fpga_interchange_arch_utils.cpp

+    }
+}
+
+void fill_switch(t_arch_switch_inf* switch_,


I see you use template here to be compatible with t_switch_inf and t_rr_switch_inf. Maybe it is better to note in code comments for developers.

I'll add comment that process_switches_array() is template function.

tangxifan · 2022-07-11T00:12:28Z

vpr/src/route/rr_graph_fpga_interchange.cpp

+    void finish_rr_generation() {
+        for (t_rr_type rr_type : RR_TYPES)
+            if (rr_type == CHANX)
+                device_ctx_.rr_graph_builder.node_lookup().resize_nodes(grid_.height(), grid_.width(), rr_type, NUM_SIDES);


I cannot find any node registered to the node_lookup().
This may cause router failed due to the needs on node_lookup().find_node().
Can you double check?

Some example:

vtr-verilog-to-routing/vpr/src/route/rr_graph2.cpp

Line 1076 in b7a94b9

rr_graph_builder.node_lookup().add_node(inode, chan, start, type, track);

As far as I know function finish_rr_generation() is responsible for building node_lookup when running loop in lines 1771 to 1774. Calling add_mode_to_all_locs(node) should be enough to register node in lookup.

Got it. Sorry for missing the details.

Signed-off-by: Maciej Dudek <[email protected]>

mtdudek · 2022-07-12T12:27:15Z

@tangxifan RR graph correctness is checked by running check_rr_graph() at the end of build_rr_graph_fpga_interchange.
As far as I understand RR edges are CHANX/CHANY, site to CHANs connections. The main function that process fpga_interchange nodes to CHAN(X|Y)s is process_nodes, it iterates over FPGA Interchange nodes and then computes virtual placements. Later in virtual_to_real virtual mapping is resolved and finally in pack_chans_edges CHANS are packed into RR graph.

tangxifan

@mtdudek Changes look good to me. Just need some code comments to be added.

vaughnbetz

Is there a spot I can look for the overall comment (big one) on how the architecture interchange format works with vtr, and for pointers to the arch interchange format documentation? I don't see any such large scale comment in this PR -- is it already in the code somewhere else? If so, please point me at it, and if not, please add such a comment.

I suspect that some more high-level commenting on the overall arch interchange interface (what is getting built when) is needed, along with pointers to bel, pip etc. definitions and how they are used in the architecture interchange format. But maybe those are already in the code base and I just am not familiar with where to look.

vaughnbetz · 2022-08-15T22:10:09Z

vpr/src/route/rr_graph_fpga_interchange.cpp

+
+        device_ctx_.rr_graph_builder.init_fan_in();
+
+        // Creates a temporary copy to convert from vtr::vector to sitd::vector.


typo:
std::vector
Should also file an issue to track changing alloc_and_load_rr_indexed_data to a vtr::vector

vaughnbetz · 2022-08-15T22:12:04Z

vpr/src/route/rr_graph_fpga_interchange.h

+                                     int* wire_to_rr_ipin_switch,
+                                     std::string& read_rr_graph_filename,
+                                     bool do_check_rr_graph);
+


Add comment saying we need to hash tuples (and why) and that the following utility functions achieve that.

vaughnbetz · 2022-08-15T22:15:06Z

vpr/src/route/rr_graph_fpga_interchange.cpp

+    VCC,
+};
+
+enum node_sides {


Comment what these are used for. Side of a block that an rr_node is on? Or something else?

vaughnbetz · 2022-08-15T22:17:13Z

vpr/src/base/read_options.cpp

@@ -1480,7 +1480,9 @@ argparse::ArgumentParser create_arg_parser(std::string prog_name, t_options& arg
        .help(
            "File format for the input atom-level circuit/netlist.\n"


This comment looks wrong -- it's not the atom-level circuit/netlist but the arch file we're talking about here.

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

acomodi force-pushed the acom/fpga-interchange-rr-graph+constants branch from 995f575 to ed6bbd6 Compare March 21, 2022 15:06

acomodi changed the title ~~WIP: FPGA interchange: add RR graph generation~~ FPGA interchange: add RR graph generation Mar 21, 2022

acomodi requested review from tangxifan and vaughnbetz March 22, 2022 09:49

acomodi force-pushed the acom/fpga-interchange-rr-graph+constants branch 3 times, most recently from ed6bbd6 to e686856 Compare April 5, 2022 14:19

mtdudek and others added 18 commits May 2, 2022 09:29

Initial FPGA Interchange rr_graph builder

46af2d1

Signed-off-by: Maciej Dudek <[email protected]>

RR_graph building for fpga interchange

8dec6b0

Signed-off-by: Maciej Dudek <[email protected]>

Small bug fixes

b3bc567

Signed-off-by: Maciej Dudek <[email protected]>

Fixes in rr graph generation

eab6266

Code refactor Signed-off-by: Maciej Dudek <[email protected]>

Fix unit test as testarch has 2 lut bels

7fc76c6

Small improvements to RR graph generation Signed-off-by: Maciej Dudek <[email protected]>

Get Xilinx 7 series to work

17298bc

Signed-off-by: Maciej Dudek <[email protected]>

Reducing rr_node and rr_edge count in fpga_interchange node translati…

ceaf604

…on step Combine node forest to single tree Signed-off-by: Maciej Dudek <[email protected]>

Add support for constant networks

96a2520

Update to new mainline Signed-off-by: Maciej Dudek <[email protected]>

Format code

eb17d62

Signed-off-by: Maciej Dudek <[email protected]>

Misc fixes and comments

4153d2b

Signed-off-by: Maciej Dudek <[email protected]>

Move common code to fpga_interchange_arch_utils

5bc078d

Signed-off-by: Maciej Dudek <[email protected]>

vpr: interchange: rr graph: use only routing segments

94d2164

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

interchange: rr graph: adapt to use new RR graph library

09c771c

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

interchange: rr graph: cleaning code and adding comments

9264618

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

interchange: arch: add alternative sites types

59a0c7e

Signed-off-by: Maciej Dudek <[email protected]>

interchange: fix alternative sites

b7745fd

Also fixes RR Graph generation with alternative site types Signed-off-by: Maciej Dudek <[email protected]>

run make format

f2ae4e5

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

acomodi force-pushed the acom/fpga-interchange-rr-graph+constants branch from e686856 to f2ae4e5 Compare May 2, 2022 07:30

kboronski-ant mentioned this pull request May 5, 2022

XDC placement constraints support for interchange #2021

Open

7 tasks

vaughnbetz requested review from amin1377 and vaughnbetz and removed request for vaughnbetz June 23, 2022 15:14

tangxifan requested changes Jul 11, 2022

View reviewed changes

Add comments

cc10573

Signed-off-by: Maciej Dudek <[email protected]>

tangxifan mentioned this pull request Jul 21, 2022

Move rr_graph_reader & rr_graph_writer to librrgraph. #2101

Merged

7 tasks

tangxifan reviewed Jul 22, 2022

View reviewed changes

vaughnbetz requested changes Aug 15, 2022

View reviewed changes

oscarcheng105 mentioned this pull request Sep 2, 2022

[WIP] Librrgraph unit test #2150

Open

7 tasks

	public: /* Validators */
	/** brief Validate that edge data is partitioned correctly
	* @note This function is used to validate the correctness of the routing resource graph in terms
	* of graph attributes. Strongly recommend to call it when you finish the building a routing resource
	* graph. If you need more advance checks, which are related to architecture features, you should
	* consider to use the check_rr_graph() function or build your own check_rr_graph() function. */
	inline bool validate_node(RRNodeId node_id) const {
	return node_storage_.validate_node(node_id, rr_switch_inf_);
	}


		auto timing_data = ar_.getPipTimings();
		arch_->num_switches = seen.size() + 2;

	static void ProcessSegments(pugi::xml_node Parent,
	std::vector<t_segment_inf>& Segs,
	const t_arch_switch_inf* Switches,
	const int NumSwitches,
	const bool timing_enabled,
	const bool switchblocklist_required,
	const pugiutil::loc_data& loc_data) {


		device_ctx_.rr_graph_builder.init_fan_in();

		// Creates a temporary copy to convert from vtr::vector to sitd::vector.

		@@ -1480,7 +1480,9 @@ argparse::ArgumentParser create_arg_parser(std::string prog_name, t_options& arg
		.help(
		"File format for the input atom-level circuit/netlist.\n"

FPGA interchange: add RR graph generation #1999

Are you sure you want to change the base?

FPGA interchange: add RR graph generation #1999

Uh oh!

Conversation

acomodi commented Mar 16, 2022

Description

Motivation and Context

Types of changes

Checklist:

Uh oh!

acomodi commented May 5, 2022

Uh oh!

vaughnbetz commented Jun 23, 2022

Uh oh!

vaughnbetz commented Jun 23, 2022

Uh oh!

tangxifan commented Jun 24, 2022

Uh oh!

tangxifan 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!

Choose a reason for hiding this comment

Uh oh!

mtdudek commented Jul 12, 2022

Uh oh!

tangxifan left a comment

Choose a reason for hiding this comment

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!

Uh oh!