Override edge attributes in RR graph #2930

soheilshahrouz · 2025-03-12T20:25:25Z

Adds a new command line option to override attributes of specific edges in the RR graph.

Description

The user can pass a text file using --read_rr_edge_override, which modifies delay and electrical characteristics of selected edges. The expected file format:

# edge    Tdel      R       Cin       Cout    Cinternal
64812     5.9e-11   450    6.0e-16   4.82e-15   3.82e-15
9981      4.2e-11   450    6.0e-16   4.82e-15   3.82e-15
1234      7.1e-11   
4321      9.4e-11   658
(42, 64)  7.3e-11

The first line must start with # and is always ignored by the parser.
Each following line must include an RR edge ID and delay (Tdel). Other attributes (resistance, capacitances) are optional. Edges can also be specified using source/sink node pairs.

How Has This Been Tested?

By manual inspection of generated RR graph files to see if the edge attributes are overridden correctly.

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

…and pass the correct argument to load_rr_file

…_filename

soheilshahrouz · 2025-03-12T20:28:12Z

@petergrossmann21 I'd apprecite any feedback on this

amin1377

Overall, this looks good to me. I’ve added several small comments, mainly related to adding comments. :)

One more thing—I think it would be good to add a test for this PR. As we discussed, testing this requires additional functionality beyond what the current infrastructure provides. I suggest creating an issue to track it, and we can implement it in a separate PR.

amin1377 · 2025-03-12T21:05:48Z

libs/librrgraph/src/base/rr_graph_builder.h

@@ -264,6 +264,10 @@ class RRGraphBuilder {
        node_storage_.alloc_and_load_edges(rr_edges_to_create);
    }

+    inline void override_edge_switch(RREdgeId edge_id, RRSwitchId switch_id) {


To maintain consistency with other functions, I think it would be better to add a Doxygen comment. (just add a brief description)

amin1377 · 2025-03-12T21:14:47Z

libs/librrgraph/src/base/rr_graph_storage.h

@@ -731,6 +730,8 @@ class t_rr_graph_storage {
     */
    void partition_edges(const vtr::vector<RRSwitchId, t_rr_switch_inf>& rr_switches);

+    void override_edge_switch(RREdgeId edge_id, RRSwitchId switch_id);


DoxyGen comment (@brief only) similar to other functions

amin1377 · 2025-03-12T21:15:23Z

libs/librrgraph/src/base/rr_graph_view.h

@@ -413,6 +413,10 @@ class RRGraphView {
        return node_storage_.edge_switch(id, iedge);
    }

+    inline short edge_switch(RREdgeId id) const {


amin1377 · 2025-03-12T21:23:15Z

libs/librrgraph/src/io/rr_graph_reader.cpp

+            }
+        }
+
+        if (!edge_id.is_valid()) {


To make it more concise, I suggest using VTR_LOGV_ERROR(expr, ...).

amin1377 · 2025-03-12T21:26:51Z

libs/librrgraph/src/io/rr_graph_reader.h

                  bool read_edge_metadata,
                  bool do_check_rr_graph,
                  bool echo_enabled,
                  const char* echo_file_name,
                  bool is_flat);

+void load_rr_edge_overrides(std::string_view filename,


Doxygen comment

amin1377 · 2025-03-12T21:27:07Z

libs/librrgraph/src/io/rr_graph_reader.cpp

+                            RRGraphBuilder& rr_graph_builder,
+                            const RRGraphView& rr_graph) {
+    std::ifstream file(filename.data());
+    if (!file) {


VTR_LOGV_ERROR

amin1377 · 2025-03-12T21:36:29Z

libs/librrgraph/src/io/rr_graph_reader.cpp

+    return edge_id;
+}
+
+struct t_rr_switch_inf_hash {


I think a better way to implement a hash function for t_rr_switch_inf is to define the hash function within the struct itself. This ensures that if someone adds a new field to the data structure, they can update the hash function directly in the same place.

Additionally, you can add the following specialization in the same file:

namespace std { template <> struct hash<t_rr_switch_inf> { size_t operator()(const t_rr_switch_inf& s) const { return s.hash(); } }; }

An added benefit of this approach is that you wouldn't need to explicitly pass the hash function when using data structures like std::unordered_map or std::unordered_set, as it will be automatically recognized.

amin1377 · 2025-03-12T21:37:28Z

libs/librrgraph/src/io/rr_graph_reader.cpp

+    }
+};
+
+struct t_rr_switch_inf_equal {


To ensure that any additional fields added to this data structure are properly handled, I suggest overloading the operator within the struct itself.

amin1377 · 2025-03-12T21:43:07Z

libs/librrgraph/src/io/rr_graph_reader.cpp

+        VTR_LOG_ERROR("Failed to open the RR edge override file: %s\n", filename.data());
+    }
+
+    std::unordered_map<t_rr_switch_inf, RRSwitchId, t_rr_switch_inf_hash, t_rr_switch_inf_equal> unique_switch_info;


I also needed to do something similar when creating edges while collapsing RR Node chains in the flat router. If you could create an issue to remind me to implement a function for this, that would be helpful.

amin1377 · 2025-03-12T21:51:22Z

libs/librrgraph/src/io/rr_graph_reader.cpp

+            RRSwitchId new_switch_id;
+            auto it = unique_switch_info.find(switch_override_info);
+            if (it == unique_switch_info.end()) {
+                new_switch_id = rr_graph_builder.add_rr_switch(switch_override_info);


Now that I think about it, I'm pretty sure I wrote a function that retrieves switch info, searches the RR Node switches for one with similar characteristics, and, if none is found, creates a new one and returns the corresponding RR switch ID (I think the name was something like get_or_create_rr_switch_id). In the issue I mentioned, if you could reference this function, I'll update this part of the code myself.

petergrossmann21 · 2025-03-13T00:20:01Z

@soheilshahrouz Some initial thoughts:

I think it might be beneficial to separate RC parameters from Tdel parameters. While it's nice to lay out all the parameters together, I don't think per-edge RC annotation makes sense. R and C are node properties rather than edge properties (see https://docs.verilogtorouting.org/en/latest/vpr/file_formats/#routing-resource-graph-file-format-xml) . So even if both parameter types are specified in a single file, I would recommend capturing them on separate lines of text, each with their own syntax. Separate files would also be fine, if slightly less elegant (see aside below)
The (src_node #, sink_node #) notation is definitely useful, as it addresses scenarios where the generator of the file does not have integer IDs for edges available during delay annotation (this happens to be the case for me, but is more generically useful because the node IDs can be reliably associated with the nets in the FPGA circuit that are used in delay capture, whereas I think edge ID associations are less reliable).
A quick sanity check on file format size: in the general case, there will be some number of bytes per edge to specify the edge ID and delay parameters. I currently develop relatively small, routing-rich architectures and am typically working with node counts at O(10K) and edge counts at O(100K). If 6 digits of precision are supplied per delay parameter, plus five characters to format in scientific notation ('.', 'e-XX'), then each delay parameter formatted in (src_node #, sink_node #) notation uses up to 27 bytes (5 for each node ID, 5 more to format them and space them apart from the delay, and then 11 for the delay constant and one for the carriage return). For a 500K-edge graph that works out to 13.5 MB -- not small, but not unworkable. This supports an architecture with O(1K) LUTs. for something with O(100K) LUTs, we would expect the file to be 100X larger, i.e. O(1GB).

I think the file size analysis above is sufficient to motivate implementing some method of shorthand in the file format so that the architect can express when multiple edges have the same delay parameter. My first instinct is that some kind of relationship between the physical tile definitions and the graph edges can help with this. Currently, the way that I do that is by relating switches in the <switchlist> section of the architecture XML to different physical tile types in my XML generator.

An aside: Note that in automatic place and route flows for ASICs, the R and C values are either not generated at all (because the wire delay is lumped with the "intrinsic" delay of the logic gates and so the VPR model will set all the RC values to 0) OR the intrinsic delay is captured independently of the RC values and thus it's convenient to annotate them separately anyway.

petergrossmann21 · 2025-03-19T15:14:32Z

@soheilshahrouz one additional thought: if the delay annotation is to be performed as a standalone step, having a dedicated executable utility to run it will be helpful--this will avoid having to call vpr for this step and having to pass in an arbitrary blif file.

soheilshahrouz added 7 commits February 7, 2025 12:49

added load_rr_edge_overrides function

ff48e6b

add -read_rr_edge_override command line option

5722c79

Merge branch 'master' into ingest_per_edge_delay

dc89abd

add help message for --read_rr_edge_override

08538ff

remove unused header files and types in rr_graph.cpp

3be8914

rename device_ctx.read_rr_graph_filename to loaded_rr_graph_filename …

7c6a282

…and pass the correct argument to load_rr_file

doxygen comment for t_det_routing_arch. Comment read_rr_edge_override…

16870df

…_filename

github-actions bot added VPR VPR FPGA Placement & Routing Tool libarchfpga Library for handling FPGA Architecture descriptions lang-cpp C/C++ code labels Mar 12, 2025

soheilshahrouz mentioned this pull request Mar 12, 2025

[WIP] Per-edge delays #2880

Closed

soheilshahrouz requested a review from amin1377 March 12, 2025 20:27

add declaration for process_rr_edge_override()

43bde82

amin1377 requested changes Mar 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Override edge attributes in RR graph #2930

Override edge attributes in RR graph #2930

soheilshahrouz commented Mar 12, 2025 •

edited

Loading

soheilshahrouz commented Mar 12, 2025

amin1377 left a comment

amin1377 Mar 12, 2025

amin1377 Mar 12, 2025

amin1377 Mar 12, 2025

amin1377 Mar 12, 2025

amin1377 Mar 12, 2025

amin1377 Mar 12, 2025

amin1377 Mar 12, 2025

amin1377 Mar 12, 2025

amin1377 Mar 12, 2025

amin1377 Mar 12, 2025

petergrossmann21 commented Mar 13, 2025

petergrossmann21 commented Mar 19, 2025

Override edge attributes in RR graph #2930

Are you sure you want to change the base?

Override edge attributes in RR graph #2930

Conversation

soheilshahrouz commented Mar 12, 2025 • edited Loading

Description

How Has This Been Tested?

Types of changes

Checklist:

soheilshahrouz commented Mar 12, 2025

amin1377 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petergrossmann21 commented Mar 13, 2025

petergrossmann21 commented Mar 19, 2025

soheilshahrouz commented Mar 12, 2025 •

edited

Loading