Data structure rr_node_metadata/rr_edge_metadata owner change #1952

Raza-jafari-RS · 2022-01-11T09:34:05Z

Description

In this PR the owner of data structures rr_node_metadata/rr_edge_metadata has been changed from device_ctx to rr_graph_builder.

Related Issue

The metadata is an internal data structure used in rr_graph_builder so it should be part of this class

Motivation and Context

These changes are continuation of reorder_nodes function location change

How Has This Been Tested?

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

tangxifan

@Raza-jafari-RS Some comments to address. Thanks for the hard working. We are getting close. If you have any questions, feel free to reach out to me.

tangxifan · 2022-01-19T19:00:37Z

vpr/src/base/vpr_context.h

@@ -149,6 +149,25 @@ struct DeviceContext : public Context {
     */
    t_rr_graph_storage rr_nodes; // autogenerated in build_rr_graph

+    /**


These comments are related to metadata. They should be moved to RRGraphBuilder and RRGraphView
These dead codes should be removed once this PR is ready to merge.

vpr/src/device/rr_graph_builder.cpp

vpr/src/device/rr_graph_builder.h

tangxifan · 2022-01-19T19:05:58Z

vpr/src/device/rr_graph_builder.h

+    inline void rr_node_metadata_clear() {
+        rr_node_metadata_.clear();
+    }
+    inline void rr_edge_metadata_clear() {


Our naming convention is clear_rr_edge_metadata()

tangxifan · 2022-01-19T19:09:26Z

vpr/src/device/rr_graph_builder.h

+        return rr_node_metadata_.size();
+    }
+
+    //rr_node_metadata_find


Templates should not be placed with regular APIs. My suggestion is to move to the head of a class definition.
See example:

vtr-verilog-to-routing/vpr/src/device/rr_graph_obj.h

Lines 213 to 224 in 36789e4

public: /* Types */

/* Iterators used to create iterator-based loop for nodes/edges/switches/segments */

typedef vtr::vector<RRNodeId, RRNodeId>::const_iterator node_iterator;

typedef vtr::vector<RREdgeId, RREdgeId>::const_iterator edge_iterator;

typedef vtr::vector<RRSwitchId, RRSwitchId>::const_iterator switch_iterator;

typedef vtr::vector<RRSegmentId, RRSegmentId>::const_iterator segment_iterator;

/* Ranges used to create range-based loop for nodes/edges/switches/segments */

typedef vtr::Range<node_iterator> node_range;

typedef vtr::Range<edge_iterator> edge_range;

typedef vtr::Range<switch_iterator> switch_range;

typedef vtr::Range<segment_iterator> segment_range;

If I make the template as a class template over the RR_graph_builder.h as it is on the metadata_storage.h. Almost all the API's in the class become unknown to the locations where they have been used. Should I make the changes in all the API's or I am doing some thing wrong as I am not worked much with the templates.

vtr-verilog-to-routing/vpr/src/base/metadata_storage.h

Lines 18 to 19 in 36789e4

template<typename LookupKey>

class MetadataStorage {

Please guide me how to resolve this issue.

tangxifan · 2022-01-19T19:12:16Z

vpr/src/route/rr_graph.cpp

@@ -1367,9 +1367,9 @@ void free_rr_graph() {

    device_ctx.switch_fanin_remap.clear();

-    device_ctx.rr_node_metadata.clear();
+    device_ctx.rr_graph_builder.rr_node_metadata_clear();


I suggest to

Convert the rr_node_metadata_clear() and rr_edge_metadata_clear() function to private.

Call these clear functions in the clear() API of RRGraphBuilder

Then, here you just need to call device_ctx.rr_graph_builder.clear()

I have called the rr_node_metadata_.clear() inside the RRGraphBuilder::clear. I have done this following the convention used for node_loopup_.clear()
https://github.com/RapidSilicon/vtr-verilog-to-routing/blob/05dd5c00e92c2cd0ff958f750be9318a05973a2c/vpr/src/device/rr_graph_builder.cpp#L62-L66

tangxifan · 2022-01-19T19:14:56Z

vpr/src/route/rr_graph_reader.cpp

@@ -72,8 +72,8 @@ void load_rr_file(const t_graph_type graph_type,
        device_ctx.rr_segments,
        device_ctx.physical_tile_types,
        grid,
-        &device_ctx.rr_node_metadata,
-        &device_ctx.rr_edge_metadata,
+        &device_ctx.rr_graph_builder.rr_node_metadata(),


If the two APIs rr_node_metadata() and rr_edge_metadata() already return reference, you may not need the & here. Please double check.

I tried to remove the & operator here and it starts throwing error of "no known conversion for arguments" we have to make changes to the locations where this function is used. So should I go for the changings or keep it in the way as it is?

It is o.k. here. We may consider to change it later.

tangxifan · 2022-01-19T19:17:25Z

vpr/src/route/rr_metadata.cpp

-    device_ctx.rr_node_metadata.add_metadata(src_node,
-                                             key,
-                                             value);
+    device_ctx.rr_graph_builder.rr_node_metadata_add_metadata(src_node,


In principle, we should not reinvent the wheel. The ideal way is

device_ctx.rr_graph_builder.rr_node_metadata().add_metadata(..)

I remember you mentioned it caused some errors in compilation. If so, I am o.k. for now.

The API rr_node_metadata_add_metadata has been removed and the add_metadata has been called as the prescribed method.

tangxifan · 2022-01-19T19:21:12Z

vpr/src/device/rr_graph_builder.h

+
+    //rr_node_metadata_find
+    template<typename LookupKey>
+    inline typename vtr::flat_map<LookupKey, t_metadata_dict>::const_iterator rr_node_metadata_find(const LookupKey& lookup_key) const {


I am reading the codes about rr_metadata at

vtr-verilog-to-routing/vpr/src/route/rr_metadata.h

Lines 8 to 14 in 36789e4

const t_metadata_value* rr_node_metadata(int src_node, vtr::interned_string key);

void add_rr_node_metadata(int src_node, vtr::interned_string key, vtr::interned_string value);

void add_rr_node_metadata(int src_node, vtr::string_view key, vtr::string_view value);

const t_metadata_value* rr_edge_metadata(int src_node, int sink_node, short switch_id, vtr::interned_string key);

void add_rr_edge_metadata(int src_node, int sink_node, short switch_id, vtr::interned_string key, vtr::interned_string value);

void add_rr_edge_metadata(int src_node, int sink_node, short switch_id, vtr::string_view key, vtr::string_view value);

It does not include templates, can you explain more details why you want to include templates?
In general, we try to avoid it because it may slow down the runtime.

I have created API's for the functions in metadata_storage.h and there a class template has been used.

vtr-verilog-to-routing/vpr/src/base/metadata_storage.h

Line 51 in 36789e4

typename vtr::flat_map<LookupKey, t_metadata_dict>::const_iterator find(const LookupKey& lookup_key) const {

Inorder to pass the values to the template I am using the function template.

tangxifan · 2022-01-20T18:13:53Z

vpr/src/device/rr_graph_builder.h

-
-    MetadataStorage<int>& rr_node_metadata_;
-    MetadataStorage<std::tuple<int, int, short>>& rr_edge_metadata_;
+    /**


we should add a warning to the comments, in order to avoid any developers to merge the metadate into other internal data:

/** .. warning:: The Metadata should stay as an independent data structure than rest of the internal data, e.g., node_lookup! */

tangxifan

@Raza-jafari-RS Thanks for the efforts. We are almost there.

tangxifan · 2022-01-21T19:05:14Z

vpr/src/device/rr_graph_builder.h

@@ -40,6 +38,37 @@ class RRGraphBuilder {
    t_rr_graph_storage& node_storage();
    /** @brief Return a writable object for update the fast look-up of rr_node */
    RRSpatialLookup& node_lookup();
+    /** @brief Return a writable object for rr_node_metadata */


Slight modification on the comments

/** @brief Return a writable object for the meta data on the nodes */

vpr/src/device/rr_graph_builder.h

tangxifan · 2022-01-21T19:09:10Z

vpr/src/device/rr_graph_view.h

@@ -403,6 +405,14 @@ class RRGraphView {
        return node_lookup_;
    }

+    MetadataStorage<int> rr_node_metadata_data() const {


Add code comments

tangxifan · 2022-01-21T19:10:05Z

vpr/src/route/rr_graph_reader.cpp

@@ -72,8 +72,8 @@ void load_rr_file(const t_graph_type graph_type,
        device_ctx.rr_segments,
        device_ctx.physical_tile_types,
        grid,
-        &device_ctx.rr_node_metadata,
-        &device_ctx.rr_edge_metadata,
+        &device_ctx.rr_graph_builder.rr_node_metadata(),


It is o.k. here. We may consider to change it later.

tangxifan

Changes are clear. Please start QoR tests.

tangxifan · 2022-01-28T18:06:26Z

@Raza-jafari-RS I have merged #1951 Please resolve the merging conflicts here. This PR is almost there. I am waiting for the QoR test results.

Raza-jafari-RS · 2022-01-31T13:21:46Z

@tangxifan ,
Sorry for the delay, Actually the master was updated and to resolve conflicts i made changes which caused a bit mess and I have to redo the work. I will upload the qor test by tomorrow.

tangxifan · 2022-01-31T18:30:25Z

@tangxifan , Sorry for the delay, Actually the master was updated and to resolve conflicts i made changes which caused a bit mess and I have to redo the work. I will upload the qor test by tomorrow.

O.K. No problem.

tangxifan · 2022-02-02T19:31:26Z

@Raza-jafari-RS Waiting for QoR test results. Code changes are good. I expect CI to be green soon

Raza-jafari-RS · 2022-02-04T10:09:45Z

QoR tests are complete and quick summary is given below. QoR comparison files are also attached.

vtr_reg_qor_chain_depop test:
Flow run time -9.09 % on average
critical path delay 0 % on average.
Peak memory usage 0.02 % on average
titan_quick_qor test:
Flow run time -15.01 % on average
critical path delay 0 % on average.
Peak memory usage 1.47 % on average

vtr_reg_qor_chain_depop.xlsx
titan_quick_qor.xlsx

tangxifan · 2022-02-04T18:18:39Z

@Raza-jafari-RS Thanks for the input and great work

@vaughnbetz I propose to merge this PR as the code changes are clean and QoR results should no degradation.
Feel free to comment. We will address any issues in future PRs.

github-actions bot added the VPR VPR FPGA Placement & Routing Tool label Jan 11, 2022

Raza-jafari-RS requested a review from tangxifan January 18, 2022 04:31

tangxifan requested changes Jan 19, 2022

View reviewed changes

tangxifan reviewed Jan 20, 2022

View reviewed changes

tangxifan requested changes Jan 21, 2022

View reviewed changes

behzadmehmood-rs requested a review from tangxifan January 25, 2022 10:23

tangxifan reviewed Jan 26, 2022

View reviewed changes

behzadmehmood-rs assigned Raza-jafari-RS Jan 31, 2022

master updated

7712eac

sfkhalid mentioned this pull request Jan 31, 2022

Kokoro tests not running on pull requests #1963

Closed

warning removal

9523219

Raza-jafari-RS requested a review from tangxifan February 1, 2022 05:24

Raza-jafari-RS added the kokoro:force-run label Feb 1, 2022

symbiflow-robot removed the kokoro:force-run label Feb 1, 2022

Raza-jafari-RS added the kokoro:force-run label Feb 2, 2022

symbiflow-robot removed the kokoro:force-run label Feb 2, 2022

after recheck

8ed65ce

Raza-jafari-RS added the kokoro:force-run label Feb 3, 2022

symbiflow-robot removed the kokoro:force-run label Feb 3, 2022

tangxifan approved these changes Feb 4, 2022

View reviewed changes

tangxifan merged commit 81b5aa2 into verilog-to-routing:master Feb 4, 2022

tangxifan deleted the metadata branch February 4, 2022 18:18

tangxifan mentioned this pull request Feb 4, 2022

Dashboard for RRGraph API refactoring status #1868

Open

44 tasks

	public: /* Types */
	/* Iterators used to create iterator-based loop for nodes/edges/switches/segments */
	typedef vtr::vector<RRNodeId, RRNodeId>::const_iterator node_iterator;
	typedef vtr::vector<RREdgeId, RREdgeId>::const_iterator edge_iterator;
	typedef vtr::vector<RRSwitchId, RRSwitchId>::const_iterator switch_iterator;
	typedef vtr::vector<RRSegmentId, RRSegmentId>::const_iterator segment_iterator;

	/* Ranges used to create range-based loop for nodes/edges/switches/segments */
	typedef vtr::Range<node_iterator> node_range;
	typedef vtr::Range<edge_iterator> edge_range;
	typedef vtr::Range<switch_iterator> switch_range;
	typedef vtr::Range<segment_iterator> segment_range;

	const t_metadata_value* rr_node_metadata(int src_node, vtr::interned_string key);
	void add_rr_node_metadata(int src_node, vtr::interned_string key, vtr::interned_string value);
	void add_rr_node_metadata(int src_node, vtr::string_view key, vtr::string_view value);

	const t_metadata_value* rr_edge_metadata(int src_node, int sink_node, short switch_id, vtr::interned_string key);
	void add_rr_edge_metadata(int src_node, int sink_node, short switch_id, vtr::interned_string key, vtr::interned_string value);
	void add_rr_edge_metadata(int src_node, int sink_node, short switch_id, vtr::string_view key, vtr::string_view value);

Data structure rr_node_metadata/rr_edge_metadata owner change #1952

Data structure rr_node_metadata/rr_edge_metadata owner change #1952

Uh oh!

Conversation

Raza-jafari-RS commented Jan 11, 2022

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

tangxifan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

tangxifan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tangxifan left a comment

Choose a reason for hiding this comment

Uh oh!

tangxifan commented Jan 28, 2022

Uh oh!

Raza-jafari-RS commented Jan 31, 2022

Uh oh!

tangxifan commented Jan 31, 2022

Uh oh!

tangxifan commented Feb 2, 2022

Uh oh!

Raza-jafari-RS commented Feb 4, 2022

Uh oh!

tangxifan commented Feb 4, 2022

Uh oh!

Uh oh!