RR Graph Overlay Creation and Deploy new node_type() API of Overlay #1693

tangxifan · 2021-03-24T21:26:52Z

This PR kicks off the refactoring effort of the routing resource graph -related data structures.
A detailed technical plan can be found at link

The overall refactoring effort aims to

create a unified data structure RRGraphView as a centralized storage for all the routing resource graph -related information. See Fig. 1
create a set of frame view object from the centralized storage RRGraph for client functions, which are suitable for rr_graph builder, placer, router, GUI, timing analyzer etc. See Fig. 2.
This is to avoid massive updates to the codes of client functions when there is a change on the unified data structure.
Also it avoid large memory footprint for client functions, since each client function may only use a small portion (typically <50%) APIs of the unified object.

Fig. 1 Illustration of the relationship between data structures

Fig. 2 Different levels of frame views of routing resource graphs to satisfy various needs from client functions.

The result/benefits of the refactoring efforts is

The routing resource graph can be decoupled from VPR's core engine as a library. Unit tests can be enabled
It is much easier for developers to develop custom routing resource graph builders thanks to the APIs of the unified data structure RRGraph. A routing resource graph builder can be a library, decoupled from VPR's core engine. Many checking codes can be efficiently merged into the data structure and developers can save a lot of efforts in writing the atom-level sanity checks.
Ensure that each client functions have a clean view of the routing resource graph, i.e. RRGraphView. In other words, routing resource graph will be read-only and only accessors are exposed to client functions. Developers have no worries on developing their own placer/router etc.

Description

This PR adds:

A new object RRGraphBuilder as the writable data structure modeling a routing resource graph. It will encapsulate rr_nodes and rr_node_indices. (To keep this PR small, currently just bring rr_node_indices in. This RRGraphBuilder will be the one and only one interface for routing resource graph builder functions.
A new object RRGraphView as the read-only data structure modeling a routing resource graph. It shares the data storage as the RRGraphBuilder. This RRGraphView is designed to be a full featured frame-view for client functions. A few variants may be introduced in later pull requests, such as reduced frame-views for client functions.
The new object RRGraphBuilder and RRGraphView are added to DeviceContext and updated each time a routing resource graph is built
Develop a basic mutator in the RRGraphBuilder for updating the rr_node_indices.
Deploy the mutator in routing resource graph building function. (May reduce to applying an example code change to validate the new object if massive code changes are required)
Develop a basic accessor in the RRGraphBuilder and RRGraphView for data queries on rr_node_indices.
Deploy the accessor in placer
Deploy the accessor in router (Deferred if massive code changes are required)

Related Issue

Motivation and Context

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

…this overlay aims to provide full featured RRGraph APIs.

…rilog-to-routing into rr_graph_node_type_overlay

…iles with minor code format fix

…nsion RRGraphBuilderView (mutable)

…mputer

tangxifan · 2021-03-26T22:19:45Z

@vaughnbetz @litghost
I have updated the description of this PR and applied code changes, according to our discussion.
Please note that:

I have not added the RRGraphBuilderView to DeviceContext because currently rr_node_indices builder creates a local copy of indices. Therefore, the RRGraphBuilderView is created locally.
I have not added fancy methods for node fast look-up builder, such as deposit an array of default node ids, which can simplify the current rr_node_indices builders. This is to avoid massive code changes. Once we agree on the data structures, it is easy to do so.

litghost · 2021-04-01T17:36:50Z

vpr/src/device/rr_graph_builder_view.h

+ * - This is the only data structre allowed to modify a routing resource graph
+ *
+ */
+class RRGraphBuilderView : public RRGraphView {


No inheritance of classes with non-abstract methods.

Second that.

Inheritance is reaaaally rarely used in real world code as it has all kinds of issues (fragile base class, hard to figure out what is going on in a stack of classes.

_{In the early days of object oriented programming, everyone thought that was a good idea, but, turns out composition is what is mostly reflecting what we want and inheritance is only really useful to describe abstract things and their implementation (meaning: the concept of interface and implementation)).}

Base classes are typically only done with interfaces; that is when the base class defines an API of sorts, but with all abstract virtual methods; then there are implementations that implement that (this is what Keith said with the comment above: unless we use the concept of implementing an interface, don't use inheritance).

vaughnbetz · 2021-04-01T18:08:25Z

April 1 discussion:

Make a class that encapsulates RRSpatialLookup
RRGraphView: has reference to this class, can return a const&. Callers who want fast lookups can call the class using that const rrSpatialLookup&
RRGraphBuilder: will also have a reference to the RRSpatialLookup data structure. It will mediate changes to the RRSpatialLookup; they'll be made as nodes are added or deleted. Could use dirty bits and bulk updates, or immediate updates. Your call, as implementation needs dictate.

hzeller

Hi, let me introduce myself: I am Henner, working with @litghost on Symbiflow FPGA stuff. Since Keith is going to work on other interesting things and can't be as much involved, I'll be helping with guidance and reviews on this project.
I'll be in the next meetings, so then I'll say hello in person :)

WRT to this project, I still have to go through all the docs; in general we probably want to go in small steps, abstracting one aspect at a time, providing accessors instead of direct access to members etc. Small pull requests (100-250 lines max) are probably good stepping stones to transform the code-base into the right direction. Starting from transforming the 'consumer' part of the RR-graph to have more abstract ways to access it, then allows in further steps to address the builder to provide things we need.
Once I get into the project I can help guide towards a robust and future-compatible API.

So below, the comments are mostly directly the C++ specific comments that come to mind immediately and that are somewhat independent of wherever the project goes. I hope they can be valuable feedback even if the scope will change. Not using inheritance unless for interface implementations, initializing things as much as possible in the constructor initializer list, sorting headers, IWUY etc. are universal good practices that can be applied right away.

hzeller · 2021-04-02T17:49:56Z

vpr/src/device/rr_graph_builder_view.cpp

+ * Mutators
+ ****************************/
+void RRGraphBuilderView::add_node_to_fast_lookup(const RRNodeId& node,
+                                                 const int& x,


Using references for simple integer types is not very useful. Only use references when you have to.

(problem is not only that there is a pointer instead of a shorter immediate type to be passed (and thus not only have to transfer 64 bits instead of 32 in this case, but if you invoke it with a number, the compiler has to generate an intermediate object that has an address), but also, that the compiler needs to often generate special code to make sure not to run into aliasing issues, which you don't have if you just copy things).

So integral values (int, char, int64_t, size_t, ...) always should be passed by value if you just have it as an in-parameter. Also for things such as std::string_view things are faster and better by value not const reference.

Agree. Updated the codes

hzeller · 2021-04-02T19:43:33Z

vpr/src/base/vpr_context.h

@@ -9,6 +9,7 @@
 #include "vtr_vector.h"
 #include "atom_netlist.h"
 #include "clustered_netlist.h"
+#include "rr_graph_view.h"


I'd suggest to always sort all headers alphabetically.

That way it is harder possible to create merge conflicts if everyone does the same.
(I realize that this is not how the practice was in this file, but I suggest to adopt that practice. It also sometimes helps finding hidden dependencies in which header files only work when included in a particular sequence)

So in emacs that would be: mark the block with all includes then M-x sort-lines; other editors have a similar feature.

In general it is adivsable to follow the practices like in this style guide: grouping blocks of c-headers, c++ headers and project headers.

(Might be worth-while to have a separate cleanup-run of this, not necessarily in this pull request. So consider this an FYI with future influence)

It makes sense to me. I will avoid modifying the vpr_context.h because it is not the focus of this PR. But I will force the alphabetical order of header files in newly created files in this PR.

hzeller · 2021-04-02T21:09:17Z

vpr/src/device/rr_graph_builder_view.cpp

+ * Constructors
+ ****************************/
+RRGraphBuilderView::RRGraphBuilderView(t_rr_graph_storage* node_storage,
+                                       t_rr_node_indices* rr_node_indices) {


Always use constructor initializer list for simple assigning of things wherelver possible. The constructor body should only really be used if we have some work that needs to be done (which, incidently, we want to avoid as much as possible in constructors, because we don't have a way to fail in a constructor - except exceptions of course, which we of course want to avoid).

So

RRGraphBuilderView::RRGraphBuilderView(t_rr_graph_storage* node_storage, t_rr_node_indices* rr_node_indices) : node_storage(node_storage), rr_node_indices_(rr_node_indices) { }

Ah, and I see, in this case you're actually initializing values from a base class. Never do that, only access the base class through its constructor.

So the base-class should have RRGraphView::RRGraphView(... these nodes ...) : node_storage(...), rr_node_indeices(...) {}.

And then here, you initialize it with

RRGraphBuilderView::RRGraphBuilderView(t_rr_graph_storage* node_storage, t_rr_node_indices* rr_node_indices) : RRGraphView(node_storage, rr_node_indices) { }

... but see also other comments about inheritance. This is probably not what you want anyway.

Thanks for the comment. I was looking for something like this and failed to find related tutorials. I have updated the codes.

hzeller · 2021-04-02T21:12:07Z

vpr/src/device/rr_graph_builder_view.h

+    RRGraphBuilderView(t_rr_graph_storage* node_storage,
+                       t_rr_node_indices* rr_node_indices);
+
+    /****************


I'd avoid the comments such as /* constructors ... */ /*mutators ...*/ that don't provide additional useful information the reader wouldn't already notice: it just adds more vertical space someone has to scroll through to get to the interesting parts (also in other files in this pull request).

Instead, focus on comments on each constructor or method. What are the parameters for these, how are these important, what does or provide the method. These are the relevant parts for someone later using the API.

The organization was actually suggested when prototyping the data structure RRGraphObject. We want to guide developers quickly to the constructors and accessors, which the developers care most. I am open to discussion still. This is mainly due to the suggested organization is not clear to me yet. We can have more discussion.

hzeller · 2021-04-02T21:22:05Z

vpr/src/device/rr_graph_builder_view.h

+     * Mutators
+     ****************/
+  public:
+    /* Register a node in the fast look-up */


Each of the parameters need to be explained. This is a public interface, so that is what users of this API will look at to know what to do. Use Doxygen-style commenting style, in which the paramter is in quotes.
Something like

Add "node" at position ("x","y"). The "type" is .. blabla whatever needs to be described here

The name of the function looks like it mixes intent ("add_node()") and implementation detail : to_fast_lookup - this is something that is up to the implementation in which way it will add the nodes (one would hope for always fast look-up I presume :)).

This is a good name for a private method name as there you have more of the implementation details that are important. The public interface should only contain names that describe what we expect from something this class should do.

Indeed. I will see what comments to added.

hzeller · 2021-04-02T22:17:56Z

vpr/src/device/rr_graph_view.h

+    /****************
+     * internal data storage, can be accessed by parent class RRGraphBuilderView
+     ****************/
+  protected:


protected on data members is what we rarely should have to do. If the derived classes need to have access to the raw members, make protected getters, but keep the members private.

Agree. Changed to private now.

hzeller · 2021-04-02T22:19:47Z

vpr/src/route/rr_graph_uxsdcxx_serializer.h

@@ -1534,6 +1537,10 @@ class RrGraphSerializer final : public uxsd::RrGraphBase<RrGraphContextTypes> {
        VTR_ASSERT(read_rr_graph_name_ != nullptr);
        read_rr_graph_filename_->assign(read_rr_graph_name_);

+        /* Update RRGraph viewer with new data pointers */
+        rr_graph_->set_internal_data(rr_nodes_,


See comment above, but this sounds like we want to avoid that and consider making a fresh rr_graph_ with pointers that are only assigned once.

Agree. Removed.

hzeller · 2021-04-02T22:26:37Z

vpr/src/device/rr_graph_builder_view.h

@@ -0,0 +1,53 @@
+#ifndef _RR_GRAPH_BUILDER_VIEW_


Avoid leading undrescores in the include-guards as they are reserved for c/c++ compiler internals.

Agree. Updated.

hzeller · 2021-04-02T22:27:25Z

vpr/src/device/rr_graph_builder_view.h

+#ifndef _RR_GRAPH_BUILDER_VIEW_
+#define _RR_GRAPH_BUILDER_VIEW_
+
+#include <exception>


why do you include <exception> ? I don't see that needed anywhere below.

Agree. Removed all the unused headers.

hzeller · 2021-04-02T22:29:27Z

vpr/src/device/rr_graph_builder_view.h

+#include "vtr_strong_id_range.h"
+#include "vtr_array_view.h"
+#include "rr_graph_storage.h"
+#include "rr_graph_view.h"


Below, the types rr_graph_storage, rr_node_indices, RRNodeId, t_rr_type and e_side are used. Do we have to include all the above to get access to these ?

IWYU, but not more.

Agree. Removed all the unused headers.

…_lookup as a sub data structure under rr_graph_view and rr_graph_builder

hzeller · 2021-04-08T00:32:48Z

vpr/src/device/rr_graph_builder.h

+     ****************/
+  private:
+    /* node-level storage including edge storages */
+    t_rr_graph_storage& node_storage_;


Only use references if they are const. If you have a modifiable object, use pointers instead (essentially like you did before).

Using non-const references creates a lot of confusion, in particular if used as instance variables.

I fully agree this convention. This rule can improve code readability and help developers to know what they are doing easily. However, as you see in the figure I showed (copied to this PR as well), the builder function will be the owner of the data storages. That is why I temporarily use the reference here. It can avoid a lot of code changes once the refactoring is finished (there is no function get data directly through the node_storage in DeviceContext). My concern is that if I use pointers, it may cause many codes in client functions or inside the data structures to be changed later.

Here are my concerns and I am open to discussion.

OK with me. Eventually the ability to get the node_storage will be removed (best) or if necessary re-written to use pointers.

tangxifan · 2021-04-08T22:30:55Z

Hi @hzeller @litghost @vaughnbetz

Just a summary what we talked and what we agreed today:

Work should be done on RRGraphView to provide fast access to internal data. But it will be schedule after the current refactoring on builder functions is finished.
Current refactoring effort will not modify the data structure organizations. In the end, the internal data storage should be organized efficiently for client functions. But this will be scheduled after the current refactoring on builder functions is finished.
Keep only one issue to solve per PR
Avoid large code changes per PR

From my point of view, we can plan the refactoring in this way:

I recommend to let this PR focused on a simple RRGraphBuild and RRGraphView as well as RRSpatialLookup, as a proof of the concept about the data structures that we all agree on.
If we all agree that the RRSpatialLookup is a right way to implement the node-level fast look-up, I can deploy the RRSpatialLookup in builder functions and client functions, as follow-up PRs.

After that, we have achieved the milestone where the builder has been improved to avoid direct write/read on the lookup data structure.
And we can discuss if we should prioritize the API addition to RRGraphView or RRGraphBuild.

Looking forward to your feedbacks.

vaughnbetz · 2021-04-09T03:00:55Z

That makes sense to me.
I think a logical order is:

Create RRSpatialLookup, as a wrapper/re-factoring around the current (data-intrusive, no API) spatial lookup code.
Next I think we want RRBuilder to have an API that allows us to do what the builder code already does: add nodes, edges, do bulk updates. I'd focus on giving the functionality we already use, but with an API around it instead of direct data structure manipulation.
Start improving APIs / removing direct data structure manipulation. Pick one at a time. Might be the geometry we talked about today, might be a cleaner RRBuilder api for node and edge addition (or maybe we'll be happy with stage 2 already).

…GraphBuilder/RRSpatialLookup

…ifan/vtr-verilog-to-routing into rr_graph_node_type_overlay

tangxifan · 2021-04-12T00:26:39Z

Hi @vaughnbetz @hzeller
This PR is ready for your review now.

Added the RRSpatialLookup with an example API add_node to register a node to lookup
Added the RRGraphBuilder which owns an RRSpatialLookup object
Deployed RRGraphBuilder to a routing resource graph building function, to showcase the use and functional correctness
Added the RRGraphView which shares the storage with RRGraphBuilder object
Deployed RRGraphView to a placement function, to showcase the use and functional correctness

This PR showcased the use of the new data structures in both builder and client functions

hzeller · 2021-04-15T18:01:53Z

vpr/src/device/rr_graph_view.h

+     * Accessors
+     ****************/
+  public:
+    /* Get the type of a routing resource node */


Probably good to add a TODO comment saying that the goal is to have these accessors private at some point totally replaced with the 'questioin' kind of methods.

Yes. The TODO comment is useful. But not every API will be converted to the questionin methods. But the questionin style is preferred.

…redundant codes to short the code view

tangxifan · 2021-04-15T22:48:36Z

@hzeller I have applied your comments on the code comments.

@kmurray If you have time to review this PR, feel free to comment.
Here is a brief of the PR's contribution. Hope it can make your reviewing process easier.

Added the RRSpatialLookup with an example API add_node to register a node to lookup
Added the RRGraphBuilder to DeviceContext which owns an RRSpatialLookup object, but only provide mutators.
Deployed RRGraphBuilder to a routing resource graph building function, to showcase the use and functional correctness
Added the RRGraphView to DeviceContext which shares the storage with RRGraphBuilder object, but only provide accessors.
Deployed RRGraphView to a placement function, to showcase the use and functional correctness

…ild, RRGraphView and RRSpatialLookup

vaughnbetz · 2021-05-20T18:50:51Z

vpr/src/device/rr_graph_builder.h

+     ****************/
+  private:
+    /* node-level storage including edge storages */
+    t_rr_graph_storage& node_storage_;


OK with me. Eventually the ability to get the node_storage will be removed (best) or if necessary re-written to use pointers.

vaughnbetz · 2021-05-20T18:55:50Z

vpr/src/device/rr_spatial_lookup.cpp

+    if (type == IPIN || type == OPIN) {
+        VTR_ASSERT_MSG(side != NUM_SIDES, "IPIN/OPIN must specify desired side (can not be default NUM_SIDES)");
+    } else {
+        VTR_ASSERT(type != IPIN && type != OPIN);


Don't really need this assert, as the prior if guarantees it is true.

Right. The assertation is just a safety check. Currently, I prefer to changing it to VTR_ASSERT_SAFE, which will not be considered unless in debug mode (no runtime cost in release mode). I am also o.k. to remove it. Please let me know what you think.

OK, your call. I'm OK with VTR_ASSERT_SAFE if you prefer, although I personally don't put asserts right in an if-chain where it is obvious the assert can't fire based on the if/else condition.

vaughnbetz · 2021-05-20T18:56:36Z

vpr/src/device/rr_spatial_lookup.cpp

+        VTR_ASSERT_MSG(side != NUM_SIDES, "IPIN/OPIN must specify desired side (can not be default NUM_SIDES)");
+    } else {
+        VTR_ASSERT(type != IPIN && type != OPIN);
+        node_side = SIDES[0];


Comment why this is done. Node side not relevant for nodes that aren't IPINs or OPINS; set to an arbitrary side?

Agree. Added comments to clarify this.

vaughnbetz · 2021-05-20T18:57:16Z

vpr/src/device/rr_spatial_lookup.cpp

+     * registering a CHANX node in the look-up
+     * TODO: Once the builders is reworked for use consistent (x, y) convention,
+     * the following swapping can be removed
+     */


Agreed. I did this years ago to allow re-use of the same data structure; it is ugly!

vaughnbetz · 2021-05-20T19:00:03Z

vpr/src/device/rr_spatial_lookup.cpp

+
+    if (node_side >= rr_node_indices_[type].dim_size(2)) {
+        return RRNodeId::INVALID();
+    }


Should add the same code to return RRNodeID::INVALID for node_x, node_y, node_side, ptc being < 0 too.

Agree. However, the node_x, node_y are already in the size_t data type, they cannot be less than 0 (converted from x , y which are int). If x is negative, when converted to size_t, it will be a huge number and out-of-range in the fast look-up. But it is really tricky. So, I added a pre-check on the x, y, node_side and ptc to return invalid ids when one of them is negative.

Ah, I didn't think of that. In that case I'm OK with just a comment noting that will happen. If you've added the pre-check for < 0 already that's fine too.

vaughnbetz · 2021-05-20T22:37:17Z

vpr/src/device/rr_spatial_lookup.h

+     * Note that for segments (CHANX and CHANY) of length > 1, the segment is
+     * given an rr_index based on the (x,y) location at which it starts (i.e.
+     * lowest (x,y) location at which this segment exists).
+     * This routine also performs error checking to make sure the node in


I'd move this comment "This routine also " to the end of the comment block.

Comment applied.

vaughnbetz · 2021-05-20T22:42:51Z

vpr/src/device/rr_spatial_lookup.h

+     *   2. track index in a routing channel when type is CHANX/CHANY
+     * - side is the side of node on the tile, applicable to OPIN/IPIN 
+     *
+     * Note that the add node here will not create a node in the node list


Note that a node added with this call will not create a node in the rr_graph_storage node list ..

Comment applied.

vaughnbetz · 2021-05-20T22:43:41Z

vpr/src/device/rr_spatial_lookup.h

+    /* Register a node in the fast look-up 
+     * - You must have a node id
+     *   1. a valid one is to register a node in the lookup
+     *   2. an invalid id means the removal of a node from the lookup


"2. an invalid id means the removal of a node from the lookup"

I don't think the .cpp file code actually does that, and it would be a strange convention. I think this part of the comment should be removed. If the comment is correct, I think we should change the convention and instead make a remove_node function.

Or perhaps this comment is intended for the data storage itself and means we can store an invalid node ID in rr_node_indices to indicate that node does not exist. That would be OK, but that comment should go with the storage declaration, not in front of the add_node function.

You are right. The use of invalid id is very tricky. It just unregisters a node from the lookup.
Actually, I should not state it because we should not promote such use of the API.
Therefore, I removed this line. We should add an API remove_node(). I put this TODO item in the comment.

vaughnbetz · 2021-05-20T22:45:20Z

vpr/src/device/rr_spatial_lookup.h

+     * The 'side' argument only applies to IPIN/OPIN types, and specifies which
+     * side of the grid tile the node should be located on. The value is ignored
+     * for non-IPIN/OPIN types
+     */


Should mention that RR_NODE_INVALID is returned if the node does not exist.

Agree. Comment applied.

I think the comment is in the wrong spot (add_node, but should be in find_node).

vaughnbetz · 2021-05-21T00:55:16Z

vpr/src/route/rr_graph2.cpp

@@ -970,8 +971,12 @@ static void load_chan_rr_indices(const int max_chan_width,
    }
 }

-static void load_block_rr_indices(const DeviceGrid& grid,
+/* As the rr_indices builders modify a local copy of indices, use the local copy in the builder 


I don't quite understand this. There is both a local couple of the rr_indices data structure and another copy in the RRGraphBuilder. It looks like some data is put in the version in the builder (with add_node) and some is in the local copy (with direct data structure access). I'm not sure where the local copy is copied back to the RRGraphBuilder copy.

It would be good to clean this up sooner rather than later. The circular dependency in the header files (rr_graph2.h etc.) should be resolvable by refactoring the header files / making other headers. Ideally we'd just use the builder functions when adding nodes to the rr_indices data structure, and would have only one copy.

This is indeed confusing. We actually share the data pointers between the rr_indices and RRGraphBuilder when initializing the DeviceContext. So the two data structures are always synchronized.

https://github.com/tangxifan/vtr-verilog-to-routing/blob/4a8443b135bdbd97d8e97da73f33ce2cedd0d7e6/vpr/src/base/vpr_context.h#L166-L180

Currently, the data ownership remains in the rr_indices because I avoid to modify many codes in rr_graph2.cpp which will make this PR tedious. I just modify a very small part to showcase that the new data structure is working fine.

And you are right. This code should be cleaned up ASAP. I also stated in the TODO of the comment in LINE 975. I will start massively deployment of the RRGraphBuilder in the function alloc_and_load_rr_indices() in next PR.

OK. I think you should comment the pointer aliasing right now though, so it's understandable how it works at this point. I didn't figure out that you'd aliased the pointers (both point at the same storage) and that's why the code worked.

vaughnbetz · 2021-05-21T00:57:25Z

@tangxifan : just finished reviewing this. Most of my feedback is minor (requests for comments or clarification), but I think there's one substantive thing: I'm not sure how the two copies of rr_indices (one in builder, one local) are synced up, and I think it would be good to get down to just one copy (in the RRGraphBuilder) sooner rather than later.

…ifan/vtr-verilog-to-routing into rr_graph_node_type_overlay

tangxifan · 2021-05-21T21:28:24Z

@vaughnbetz Thanks a lot for the constructive comments. I have updated the PR and clarify as much as I can. Feel free to bug if you see anything confusing.

vaughnbetz · 2021-05-21T22:03:37Z

vpr/src/device/rr_spatial_lookup.h

+     *   - track index in a routing channel when type is CHANX/CHANY
+     * - side is the side of node on the tile, applicable to OPIN/IPIN 
+     *
+     * An invalid id will be returned if the node does not exist


I think this comment (an invalid id will be returned if the node does not exist) is in the wrong spot. It should be with find_node, not add_node.

You are right. I moved to the find_node() API.

vaughnbetz

Looks good Xifan, thanks. I added a few follow-up comments (one comment is in the wrong spot, think pointer aliasing of rr_node_indices should be commented even though it's a temporary fix). Other than that looks good to go.

vaughnbetz · 2021-05-21T22:07:08Z

Spoke too soon: looks like CI also caught a bug in an assert:
</home/runner/work/vtr-verilog-to-routing/vtr-verilog-to-routing/vpr/src/device/rr_spatial_lookup.cpp:83 add_node: Assertion 'node' failed.

…enabled later when remove_node() API is available in rr_spatial_lookup)

tangxifan · 2021-05-21T22:45:30Z

@vaughnbetz I have patched the comment and clarify the dirty sync between rr_node_indices and rr_spatial_lookup:

https://github.com/tangxifan/vtr-verilog-to-routing/blob/49e3339488e2f02a3b07df3ec53a6fa0b7f53a43/vpr/src/base/vpr_context.h#L166-L172

I will keep watching the CI and fix anything red.

tangxifan · 2021-05-22T20:46:41Z

@vaughnbetz CI is green now. The pull request is ready for your review again.

vaughnbetz · 2021-05-25T15:06:47Z

Updating branch (pretty sure no related changes went in, so this should be fine). I'll merge as soon as the fast running tests pass and show no compile or basic errors; no need to wait for vtr_reg_nightly I think.

vaughnbetz · 2021-05-25T21:27:52Z

Everything passed except vtr nightly was in progress; updated again due to other changes. really will merge once the fast running tests are done.

vaughnbetz · 2021-05-26T02:06:59Z

The Odin-II failure is unrelated; due to moving some runs onto kokoro (in progress). This has passed vtr_nightly before so merging it now (it's been rebased twice already).

tangxifan added 4 commits March 24, 2021 13:58

[VPR] Add an overlay RRGraph object; Different from t_rr_graph_view, …

6a2eb50

…this overlay aims to provide full featured RRGraph APIs.

[VPR] Update rr_graph overlay to enable its deployment in DeviceContext

cb55acc

[VPR] Replace node type() API with overlay API in check_rr_graph.cpp

584f7fc

Merge branch 'master' of https://github.com/verilog-to-routing/vtr-ve…

b86c734

…rilog-to-routing into rr_graph_node_type_overlay

github-actions bot added the VPR VPR FPGA Placement & Routing Tool label Mar 24, 2021

tangxifan marked this pull request as draft March 24, 2021 21:32

tangxifan added 8 commits March 24, 2021 15:34

[VPR] add overlay initialization for rr_graph loading from external f…

e927bb5

…iles with minor code format fix

[VPR] Code format fix

b25adaa

[VPR] Code format fix

59b766c

[VPR] Add rr graph overlay to rr_graph reader and writer

0d697c0

[VPR] Add a prototype of RRGraphView (read-only) and its builder exte…

ea1c017

…nsion RRGraphBuilderView (mutable)

[VPR] Bug fix in RRGraphView API and deploy to placer delay matrix co…

4c82389

…mputer

[VPR] Code format fix

dcdb036

[VPR] Deploy mutator to rr_node_indice builder as an example

387eba7

tangxifan marked this pull request as ready for review March 26, 2021 22:08

Merge branch 'master' into rr_graph_node_type_overlay

6e1427b

litghost reviewed Apr 1, 2021

View reviewed changes

hzeller reviewed Apr 2, 2021

View reviewed changes

tangxifan added 3 commits April 7, 2021 11:48

Merge branch 'master' into rr_graph_node_type_overlay

409e527

[VPR] Reworked rr_graph_view and rr_graph_builder; Created rr_spatial…

32225c8

…_lookup as a sub data structure under rr_graph_view and rr_graph_builder

[VPR] code format fix; add comments to API

6ae1016

hzeller reviewed Apr 8, 2021

View reviewed changes

Merge branch 'master' into rr_graph_node_type_overlay

161d66d

tangxifan added 2 commits April 9, 2021 17:28

[VPR] Add comments to explain why references are currently used in RR…

7c0169c

…GraphBuilder/RRSpatialLookup

Merge branch 'rr_graph_node_type_overlay' of https://github.com/tangx…

a758762

…ifan/vtr-verilog-to-routing into rr_graph_node_type_overlay

hzeller reviewed Apr 15, 2021

View reviewed changes

[VPR] Reworked comments to emphasize on confusing codes while delete …

706a965

…redundant codes to short the code view

tangxifan added 3 commits April 16, 2021 10:44

[VPR] Reworked the comment about avoid copy contructors for RRGraphBu…

0297ce9

…ild, RRGraphView and RRSpatialLookup

Merge branch 'master' into rr_graph_node_type_overlay

4916c1b

Merge branch 'master' into rr_graph_node_type_overlay

4a8443b

vaughnbetz requested changes May 21, 2021

View reviewed changes

tangxifan added 2 commits May 21, 2021 11:35

Merge branch 'rr_graph_node_type_overlay' of https://github.com/tangx…

2fdabbd

…ifan/vtr-verilog-to-routing into rr_graph_node_type_overlay

[VPR] Update comments to avoid confusion and clarify

e137b6d

vaughnbetz reviewed May 21, 2021

View reviewed changes

[VPR] Patch comments and remove the inproper assert (which should be …

49e3339

…enabled later when remove_node() API is available in rr_spatial_lookup)

Merge branch 'master' into rr_graph_node_type_overlay

76b6f00

ethanroj23 mentioned this pull request May 25, 2021

Implementation Methods for RRGraphView API #1740

Closed

Merge branch 'master' into rr_graph_node_type_overlay

82da2e0

Merge branch 'master' into rr_graph_node_type_overlay

ffb4bca

vaughnbetz merged commit 1f7fd26 into verilog-to-routing:master May 26, 2021

ethanroj23 mentioned this pull request May 27, 2021

RRGraphView::node_type() Implementation #1742

Merged

7 tasks

This was referenced May 28, 2021

Deploy RRGraphBuilder in place of rr_node_indices in routing resource graph builder functions #1746

Closed

Deploy RRGraphBuilder in place of rr_node_indices in routing resource graph builder functions #1747

Merged

This was referenced Jun 16, 2021

Deploy RRGraphBuilder in Regular Routing Resource Graph Building Functions to replace the use of rr_node_indices #1777

Merged

Deploy RRGraphBuilder in Client Functions to replace the use of rr_node_indices #1779

Merged

RR Graph Overlay Creation and Deploy new node_type() API of Overlay #1693

RR Graph Overlay Creation and Deploy new node_type() API of Overlay #1693

Uh oh!

Conversation

tangxifan commented Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

tangxifan commented Mar 26, 2021

Uh oh!

litghost Apr 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vaughnbetz commented Apr 1, 2021

Uh oh!

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

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 commented Apr 8, 2021

Uh oh!

vaughnbetz commented Apr 9, 2021

Uh oh!

tangxifan commented Apr 12, 2021

Uh oh!

tangxifan commented Mar 24, 2021 •

edited

Loading

litghost Apr 1, 2021 •

edited

Loading