Adding Custom RR Graph Builder to OpenFPGA #3325
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughIntroduces Custom Routing Resources (CRR) generation to VPR, enabling data-driven switch-block configuration from YAML maps and CSV template files. Adds yaml-cpp dependency, new t_crr_opts data structure, command-line arguments, and threads CRR options through initialization, placement, and routing flows. Implements CRR generator components including pattern matching, connection building, and edge creation with template-aware switching. Extends t_arch_switch_inf and t_rr_switch_inf with template_id metadata, and integrates statistics collection for switch usage. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant VPR as VPR Setup
participant CfgMgr as SwitchBlockManager
participant PatMatcher as CRRPatternMatcher
participant NodeMgr as NodeLookupManager
participant ConnBuilder as CRRConnectionBuilder
participant EdgeBuilder as CRR Edge Builder
participant RRGraph as RR Graph Builder
User->>VPR: Initialize with --sb_maps, --sb_templates
VPR->>VPR: Parse CRR options
VPR->>CfgMgr: initialize(sb_maps_file, sb_templates_dir)
CfgMgr->>CfgMgr: Load YAML switch-block map
CfgMgr->>CfgMgr: Read and cache CSV template DataFrames
VPR->>NodeMgr: Initialize node lookup structures
NodeMgr->>NodeMgr: Index RR graph nodes by column/row
loop For each tile (x, y)
VPR->>PatMatcher: find_matching_pattern(x, y)
PatMatcher->>CfgMgr: Resolve pattern name
CfgMgr-->>PatMatcher: Return matched pattern
PatMatcher-->>VPR: Pattern found
VPR->>ConnBuilder: build_connections_for_location(x, y)
ConnBuilder->>NodeMgr: get_tile_source_nodes(x, y)
NodeMgr-->>ConnBuilder: Return node map
ConnBuilder->>NodeMgr: get_tile_sink_nodes(x, y)
NodeMgr-->>ConnBuilder: Return node map
ConnBuilder->>CfgMgr: get_switch_block_dataframe(pattern)
CfgMgr-->>ConnBuilder: Return DataFrame
ConnBuilder->>ConnBuilder: Parse DataFrame cells, compute PTc sequences
ConnBuilder->>ConnBuilder: Build Connection objects with template_id
ConnBuilder-->>VPR: Return connections
VPR->>EdgeBuilder: build_crr_gsb_edges(connections)
EdgeBuilder->>EdgeBuilder: For each connection: determine or create CRR switch
EdgeBuilder->>RRGraph: Add edge with switch_id, source, sink
RRGraph-->>EdgeBuilder: Edge added
end
RRGraph-->>VPR: RR graph construction complete
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
♻️ Duplicate comments (3)
vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/node_lookup_manager.cpp (1)
16-24: Consider documenting why lookup sizes are grid dimensions + 1.The lookup vectors are sized to
fpga_grid_x_ + 1andfpga_grid_y_ + 1. A brief comment explaining this sizing (e.g., whether it's due to 1-based indexing or boundary handling) would help future maintainers understand the design choice.+ // Lookup vectors are sized to grid dimensions + 1 to allow direct indexing + // by grid coordinates, which may include the boundary column/row. column_lookup_.resize(fpga_grid_x_ + 1); row_lookup_.resize(fpga_grid_y_ + 1);vpr/src/route/rr_graph_generation/tileable_rr_graph/tileable_rr_graph_edge_builder.cpp (1)
340-342: Add comment explaining the -2 offset.As noted in a previous review, the
-2offset for grid dimensions should be documented. This appears to exclude boundary tiles from CRR processing.Consider adding:
crr_connection_builder = std::make_unique<crrgenerator::CRRConnectionBuilder>(rr_graph, node_lookup, sb_manager); + // Initialize for internal GSBs only (exclude boundary tiles on each side) crr_connection_builder->initialize(grids.width() - 2, grids.height() - 2, crr_opts.annotated_rr_graph);vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/crr_edge_builder.cpp (1)
35-60: O(n) lookup may become a performance concern.The
find_or_create_crr_switch_idfunction iterates through all switches to find a matchingtemplate_id. As noted in a previous review, this could become quadratic if called frequently during graph construction. Consider adding a reverse lookup map (template_id→sw_id) if this proves to be a bottleneck.For now, if the number of unique CRR switches is small (< 100), this is acceptable.
🟡 Minor comments (7)
doc/src/vpr/index.rst-61-63 (1)
61-63: Update toctree entry to match the actual filenameThe toctree entry at line 63 references
custom_rr_graph_generator, but the actual file on disk iscustom_rr_graph.rst. This will cause a Sphinx warning about a missing toctree entry and result in a broken navigation link.Update the entry as follows:
- custom_rr_graph_generator + custom_rr_graphdoc/src/vpr/custom_rr_graph.rst-59-60 (1)
59-60: Fix typo and grammar.Line 59 contains a typo: "sitch type" should be "switch type". Also, "delay for that connections" should be "delay for that connection" (singular).
Apply this diff:
-* An **'x' mark** at an intersection indicates that the source and sink nodes are connected. The delay for that connections is retrieved from the sitch type specified in the architecture file. +* An **'x' mark** at an intersection indicates that the source and sink nodes are connected. The delay for that connection is retrieved from the switch type specified in the architecture file.vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/crr_connection_builder.h-138-143 (1)
138-143: Documentation mismatch: comment says "switch id" but function returns delay.The comment states "Return the switch id of an edge" but the function is named
get_connection_delay_ps, suggesting it returns a delay value in picoseconds. Please correct the comment to match the function's actual purpose.- // Return the switch id of an edge between two nodes + // Return the delay in picoseconds for a connection between two nodes int get_connection_delay_ps(const std::string& cell_value,vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/crr_edge_builder.cpp-50-54 (1)
50-54: Potential ID collision ifall_sw_infhas gaps.Using
all_sw_inf.size()as the new switch ID assumes contiguous IDs. If the map ever has gaps (e.g., from removals), this could overwrite an existing entry. Consider using the maximum existing key + 1, or verifying the ID doesn't exist before insertion.if (found_sw_id == -1) { t_arch_switch_inf new_arch_switch_inf = create_crr_switch(delay_ps, sw_template_id); - found_sw_id = static_cast<int>(all_sw_inf.size()); + // Find the next available ID (max key + 1) + found_sw_id = all_sw_inf.empty() ? 0 : (all_sw_inf.rbegin()->first + 1); all_sw_inf.emplace(found_sw_id, std::move(new_arch_switch_inf));vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/data_frame_processor.h-147-149 (1)
147-149: Inconsistent iterator comparison.
ConstRowIterator::operator!=only comparesrow_, whileRowIterator::operator!=(line 122-124) also compares thedf_pointer. This inconsistency could lead to subtle bugs when comparing iterators from different DataFrames.bool operator!=(const ConstRowIterator& other) const { - return row_ != other.row_; + return df_ != other.df_ || row_ != other.row_; }vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/data_frame_processor.h-48-56 (1)
48-56:std::stollcan throw exceptions.When
as_int()is called on a string value that cannot be converted to an integer (e.g., "abc"),std::stollwill throwstd::invalid_argumentorstd::out_of_range. Consider adding error handling or documenting this behavior.int64_t as_int() const { if (auto p = std::get_if<int64_t>(&value)) { return *p; } if (auto p = std::get_if<std::string>(&value)) { - return std::stoll(*p); + try { + return std::stoll(*p); + } catch (const std::exception&) { + return 0; + } } return 0; }vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/crr_connection_builder.cpp-309-310 (1)
309-310:std::stoican throw on malformed segment type.If
info.seg_typedoesn't follow the expected format (e.g., "L4"),substr(1)orstoicould throw. Consider adding validation or error handling.- int seg_length = std::stoi( - info.seg_type.substr(1)); // Extract number from "L1", "L4", etc. + int seg_length = 1; + if (info.seg_type.size() > 1) { + try { + seg_length = std::stoi(info.seg_type.substr(1)); + } catch (const std::exception& e) { + VTR_LOG_ERROR("Invalid segment type format: %s\n", info.seg_type.c_str()); + return RRNodeId::INVALID(); + } + }Committable suggestion skipped: line range outside the PR's diff.
🧹 Nitpick comments (19)
libs/librrgraph/src/base/rr_node_types.h (1)
59-81: Reduce duplication by leveragingRR_TYPES/rr_node_typenameand consider explicit assert includeThe current
if/elsechain is functionally correct and cheap for this small enum, but it duplicates the enum→string mapping that already exists viaRR_TYPESandrr_node_typename. This makes future additions toe_rr_typeeasy to forget here, which can lead to subtle mismatches.You can make the lookup self‑maintaining by iterating
RR_TYPESand comparing againstrr_node_typename[type]:-inline e_rr_type get_rr_type(const std::string& type_name) { - if (type_name == "SOURCE") { - return e_rr_type::SOURCE; - } else if (type_name == "SINK") { - return e_rr_type::SINK; - } else if (type_name == "IPIN") { - return e_rr_type::IPIN; - } else if (type_name == "OPIN") { - return e_rr_type::OPIN; - } else if (type_name == "CHANX") { - return e_rr_type::CHANX; - } else if (type_name == "CHANY") { - return e_rr_type::CHANY; - } else if (type_name == "CHANZ") { - return e_rr_type::CHANZ; - } else if (type_name == "MUX") { - return e_rr_type::MUX; - } else { - std::string error_message = "Invalid RR type name: " + type_name; - VTR_ASSERT_MSG(false, error_message.c_str()); - return e_rr_type::NUM_RR_TYPES; - } -} +inline e_rr_type get_rr_type(const std::string& type_name) { + for (auto type : RR_TYPES) { + if (type_name == rr_node_typename[type]) { + return type; + } + } + + std::string error_message = "Invalid RR type name: " + type_name; + VTR_ASSERT_MSG(false, error_message.c_str()); + return e_rr_type::NUM_RR_TYPES; +}This keeps the string↔enum mapping in one place and automatically picks up new
e_rr_typevalues as long asRR_TYPES/rr_node_typenameare updated.Separately, since this function introduces a direct use of
VTR_ASSERT_MSG, it would be slightly safer to ensure the corresponding header (e.g.,#include "vtr_assert.h") is explicitly included in this header or in some always‑included common header, rather than relying on transitive includes.vpr/src/route/rr_graph_generation/tileable_rr_graph/tileable_rr_graph_gsb.cpp (3)
1193-1206: Document when empty maps are expected and consider defensive checks.The guard prevents edge creation when
opin2track_mapis empty, which is reasonable for CRR scenarios where standard mappings may not be populated. However, this could silently skip edge creation if the map is unexpectedly empty.Consider:
- Adding a comment explaining when
opin2track_map.size() == 0is expected (e.g., "When CRR is enabled, opin2track_map may be empty as connections are defined by custom patterns").- Adding an assertion or debug logging to detect cases where an empty map is unexpected:
VTR_ASSERT_MSG(opin2track_map.size() > 0 || crr_enabled, "opin2track_map unexpectedly empty when CRR not enabled");Based on the AI summary, this relates to CRR integration.
1212-1224: Document when empty maps are expected and consider defensive checks.Similar to the OPIN guard, this prevents iteration when
track2ipin_mapis empty. Consider adding documentation and defensive assertions as suggested for the OPIN guard above.
1227-1235: Document when empty maps are expected and consider defensive checks.This guard follows the same pattern as the previous two. Consider adding documentation and defensive assertions to clarify when an empty
track2track_mapis expected behaviour versus an error condition.libs/librrgraph/src/base/rr_edge.h (1)
3-5: Unused includes in this header.The
<string>and<optional>headers are added but not used within this file. If these were added for transitive inclusion by dependent files, consider adding them directly where needed to keep headers self-contained and avoid unnecessary compilation dependencies.vpr/src/base/setup_vpr.cpp (1)
51-52: CRR options plumbing in SetupVPR is aligned with existing option setupAdding
t_crr_opts* crrOptstoSetupVPRand initialising it viasetup_crr_opts(*options, *crrOpts);follows the same pattern as the other option structs and keeps CRR configuration in one place. Thesetup_crr_optshelper cleanly mirrors the fields int_options(sb_maps,sb_templates,crr_num_threads, preserve flags, annotated graph, dangling removal,sb_count_dir), so the mapping is straightforward.Two minor points to keep in mind:
- As with other
*Optsarguments, all callers must now pass a valid, non-nullt_crr_opts*before this runs.- If you later need cross-field validation of CRR inputs (e.g., that
sb_mapsonly references definedsb_templates), this helper is a good central place to add it instead of scattering checks elsewhere.Also applies to: 92-115, 151-159, 761-770
libs/librrgraph/src/base/rr_switch.h (1)
34-39: New template_id field looks fine; consider its role in equality and hashingAdding
template_id(with clear documentation) is straightforward and keeps CRR-related metadata attached to eacht_rr_switch_inf.One thing to double-check elsewhere: if
t_rr_switch_inf::operator==andt_rr_switch_inf::Hasherare used in sets/maps, ensure they either intentionally include or intentionally ignoretemplate_idin a way that matches your intended semantics (i.e., whether two switches that differ only in template ID should be considered equal or not).doc/src/vpr/custom_rr_graph.rst (1)
1-176: Consider addressing previous review comments.Based on the past review comments, consider the following improvements:
- Line 3: The title could be more specific as "Custom RR Graph Generator" (as suggested by AlexandreSinger)
- Line 16: Adding a simple example YAML file would improve clarity
- Line 86: A simpler, concrete CSV example showing actual file content would help readers understand the format better than just the calculation example
- Figures: If the figures appear small, consider adjusting their width to 100%
The command-line arguments are documented at lines 168-173, which addresses that concern.
vpr/src/route/rr_graph_generation/rr_graph.cpp (1)
416-596: CRR options are correctly threaded into the tileable UNIDIR RR-graph pathThe updated
create_rr_graphsignature (const t_crr_opts& crr_opts) and the call:build_tileable_unidir_rr_graph(block_types, grid, nodes_per_chan, crr_opts, /* ... */, router_opts.route_verbosity, Warnings);ensure CRR configuration only affects the tileable unidirectional RR‑graph builder, leaving the non‑tileable and file‑loaded paths unchanged. This matches the intended “CRR only for tileable RR graphs” design and looks behaviourally safe. It might be worth documenting (or eventually warning) that CRR options are ignored when
graph_type != e_graph_type::UNIDIR_TILEABLEor when loading an RR graph from file, but that’s an optional polish.vpr/src/base/read_options.cpp (1)
3283-3323: Minor wording / clarity tweaks for new CRR CLI optionsThe new CRR flags look reasonable and well‑grouped. A couple of tiny wording fixes would make the help text clearer, e.g.:
- Lines 3301 and 3306: “If it set to on” → “If set to 'on', …”.
- Consider explicitly saying “YAML switch‑block map file” for
--sb_maps, and that--sb_templatesis a directory of CSV templates, to align with the docs.Purely textual; behaviour is fine as‑is.
vpr/src/base/stats.cpp (1)
260-323: Consider validating CRR count↔CSV correspondence and filesystem assumptionsThe overall
write_sb_count_stats()flow (collect counts pertemplate_id, group by filename viaparse_sb_key, trim CSVs fromsb_map_dir, then expand/update cells and write intosb_count_dir) is sound, but there are a couple of edge cases worth defensive handling:
- If
file_groupscontains a filename that is not present undersb_map_dir(e.g., a staletemplate_idafter renaming/removing a CSV), those counts are silently dropped. Emitting a warning or aVTR_ASSERTwhen a referenced filename is missing would help catch misconfigurations.std::filesystem::directory_iterator(sb_map_dir)and opening files undersb_count_dirassume the directories exist and are readable/writable; if they don’t, you’ll get a thrownstd::filesystem_erroror silently failed writes. A small check up front (e.g.,fs::exists/fs::is_directoryonsb_map_dirandsb_count_dir, withVPR_FATAL_ERRORorVTR_LOG_ERROR) would give users a clearer error path.These are safety/UX improvements; the current logic will work when inputs and directories are correctly set up.
vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/crr_pattern_matcher.h (1)
84-171: Pattern→regex conversion and validation logic is coherent; consider tests and potential cachingThe overall approach in
pattern_to_regex()/matches_pattern()is consistent:
*is translated to a([0-9]+)capture group.[ ... ]constructs become a numeric capture group validated later viamatches_range().- Literal regex metacharacters are escaped, and
\*is treated as a literal*.A couple of small follow‑ups to consider (non‑blocking):
- Add focused unit tests for representative patterns (
"tile_X*Y*", ranges like"[0,3,7]","[2:32:3]", and mixed patterns) plus a few malformed inputs to ensure thematches_range()/capture‑index logic behaves as expected.- If you end up calling
matches_pattern()in an inner loop over many tiles with the same small set of patterns, cachingstd::regexobjects per pattern (or at least the compiled regex string) would avoid paying the compile cost each time.Functionally this looks good; these are about robustness and potential performance tuning.
vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/node_lookup_manager.cpp (1)
45-57: Potential performance consideration for get_combined_nodes.This method creates a new
unordered_mapand copies entries from both column and row lookups on each call. For frequently accessed tiles, this could be a performance bottleneck. Consider whether caching combined results or returning a view/iterator pair might be beneficial for hot paths.vpr/src/base/vpr_types.h (1)
1413-1423: Consider adding default values for t_crr_opts members.The struct members lack default initializers. While strings default-construct to empty, the
intandboolmembers will have indeterminate values if the struct is default-constructed without explicit initialization. Adding defaults improves robustness and documents intended default behavior./// Stores CRR specific options struct t_crr_opts { std::string sb_maps; std::string sb_templates; - int crr_num_threads; - bool preserve_input_pin_connections; - bool preserve_output_pin_connections; - bool annotated_rr_graph; - bool remove_dangling_nodes; + int crr_num_threads = 1; + bool preserve_input_pin_connections = false; + bool preserve_output_pin_connections = false; + bool annotated_rr_graph = false; + bool remove_dangling_nodes = false; std::string sb_count_dir; };vpr/src/route/rr_graph_generation/tileable_rr_graph/tileable_rr_graph_builder.cpp (1)
10-11: Unused include.The
#include <fstream>header does not appear to be used in this file. If it's not needed, consider removing it.-#include <fstream> -vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/crr_common.h (1)
66-69: Returnsw_template_idby const reference to avoid unnecessary copies.The
sw_template_id()accessor returnsstd::stringby value, which creates a copy on each call. Since the string is stored as a member, returning by const reference is more efficient.- std::string sw_template_id() const { return sw_template_id_; } + const std::string& sw_template_id() const { return sw_template_id_; }vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/crr_switch_block_manager.h (1)
98-106: Clarify pointer ownership model.The
dataframes_map stores raw pointers toDataFrame, whilefile_cache_storesDataFrameobjects by value. Ifdataframes_points intofile_cache_, this is safe as long as:
file_cache_is not modified afterdataframes_is populated- The SwitchBlockManager outlives all uses of the pointers
Consider adding a comment documenting this ownership relationship, or using
std::shared_ptrif the lifetime isn't strictly controlled./** * @brief Maps switch block patterns to their corresponding dataframes. + * + * Note: These pointers reference entries in file_cache_ and are valid + * for the lifetime of this SwitchBlockManager instance. */ std::unordered_map<std::string, DataFrame*> dataframes_;vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/crr_connection_builder.cpp (1)
72-73: Consider using verbose logging for per-tile messages.This log message will be printed for every switch block location processed. For a large FPGA (e.g., 100x100), this could generate thousands of log messages. Consider using
VTR_LOGVwith a verbosity check.- VTR_LOG("Processing switch block with pattern '%s' at (%zu, %zu)\n", - pattern.c_str(), x, y); + VTR_LOG_DEBUG("Processing switch block with pattern '%s' at (%zu, %zu)\n", + pattern.c_str(), x, y);vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/data_frame_processor.cpp (1)
297-311: Integer parsing logic has edge cases.The
is_integercheck (lines 298-304) only accepts positive integers (digits only), butstd::stoion line 307 could theoretically receive a string that passed validation but is too large forint. Additionally, negative integers are not handled.Consider using a more robust parsing approach consistent with the
is_integerfunction incrr_connection_builder.cppthat usesstd::from_chars.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
doc/src/vpr/lane_and_tap.pngis excluded by!**/*.pngdoc/src/vpr/lane_and_tap_realistic.pngis excluded by!**/*.pnglibs/librrgraph/src/io/gen/rr_graph_uxsdcxx.his excluded by!**/gen/**libs/librrgraph/src/io/gen/rr_graph_uxsdcxx_capnp.his excluded by!**/gen/**libs/librrgraph/src/io/gen/rr_graph_uxsdcxx_interface.his excluded by!**/gen/**libs/libvtrcapnproto/gen/rr_graph_uxsdcxx.capnpis excluded by!**/gen/**
📒 Files selected for processing (66)
.gitmodules(1 hunks)doc/src/vpr/command_line_usage.rst(1 hunks)doc/src/vpr/custom_rr_graph.rst(1 hunks)doc/src/vpr/index.rst(1 hunks)libs/EXTERNAL/CMakeLists.txt(1 hunks)libs/EXTERNAL/yaml-cpp(1 hunks)libs/libarchfpga/src/physical_types.h(1 hunks)libs/librrgraph/src/base/rr_edge.h(1 hunks)libs/librrgraph/src/base/rr_graph_builder.cpp(1 hunks)libs/librrgraph/src/base/rr_graph_builder.h(4 hunks)libs/librrgraph/src/base/rr_graph_storage.cpp(1 hunks)libs/librrgraph/src/base/rr_graph_storage.h(2 hunks)libs/librrgraph/src/base/rr_graph_view.cpp(1 hunks)libs/librrgraph/src/base/rr_graph_view.h(1 hunks)libs/librrgraph/src/base/rr_node_types.h(1 hunks)libs/librrgraph/src/base/rr_switch.h(1 hunks)libs/librrgraph/src/io/rr_graph.xsd(1 hunks)libs/librrgraph/src/io/rr_graph_uxsdcxx_serializer.h(1 hunks)utils/route_diag/src/main.cpp(1 hunks)vpr/CMakeLists.txt(1 hunks)vpr/src/analytical_place/analytical_placement_flow.cpp(1 hunks)vpr/src/analytical_place/detailed_placer.cpp(1 hunks)vpr/src/base/place_and_route.cpp(6 hunks)vpr/src/base/place_and_route.h(1 hunks)vpr/src/base/read_options.cpp(1 hunks)vpr/src/base/read_options.h(1 hunks)vpr/src/base/setup_vpr.cpp(4 hunks)vpr/src/base/setup_vpr.h(1 hunks)vpr/src/base/stats.cpp(3 hunks)vpr/src/base/stats.h(1 hunks)vpr/src/base/vpr_api.cpp(8 hunks)vpr/src/base/vpr_api.h(1 hunks)vpr/src/base/vpr_types.h(2 hunks)vpr/src/place/delay_model/PlacementDelayModelCreator.cpp(2 hunks)vpr/src/place/delay_model/PlacementDelayModelCreator.h(2 hunks)vpr/src/place/place.cpp(2 hunks)vpr/src/place/place.h(1 hunks)vpr/src/route/route.cpp(2 hunks)vpr/src/route/route.h(1 hunks)vpr/src/route/route_utils.cpp(2 hunks)vpr/src/route/route_utils.h(1 hunks)vpr/src/route/router_delay_profiling.cpp(2 hunks)vpr/src/route/router_delay_profiling.h(1 hunks)vpr/src/route/rr_graph_generation/rr_graph.cpp(3 hunks)vpr/src/route/rr_graph_generation/rr_graph.h(1 hunks)vpr/src/route/rr_graph_generation/rr_graph_switch_utils.cpp(2 hunks)vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/crr_common.h(1 hunks)vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/crr_connection_builder.cpp(1 hunks)vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/crr_connection_builder.h(1 hunks)vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/crr_edge_builder.cpp(1 hunks)vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/crr_edge_builder.h(1 hunks)vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/crr_pattern_matcher.h(1 hunks)vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/crr_switch_block_manager.cpp(1 hunks)vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/crr_switch_block_manager.h(1 hunks)vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/data_frame_processor.cpp(1 hunks)vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/data_frame_processor.h(1 hunks)vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/node_lookup_manager.cpp(1 hunks)vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/node_lookup_manager.h(1 hunks)vpr/src/route/rr_graph_generation/tileable_rr_graph/tileable_rr_graph_builder.cpp(8 hunks)vpr/src/route/rr_graph_generation/tileable_rr_graph/tileable_rr_graph_builder.h(2 hunks)vpr/src/route/rr_graph_generation/tileable_rr_graph/tileable_rr_graph_edge_builder.cpp(8 hunks)vpr/src/route/rr_graph_generation/tileable_rr_graph/tileable_rr_graph_edge_builder.h(4 hunks)vpr/src/route/rr_graph_generation/tileable_rr_graph/tileable_rr_graph_gsb.cpp(2 hunks)vtr_flow/tasks/regression_tests/vtr_reg_strong/strong_custom_grid/config/golden_results.txt(1 hunks)vtr_flow/tasks/regression_tests/vtr_reg_strong/strong_xilinx_flagship/config/golden_results.txt(1 hunks)vtr_flow/tasks/regression_tests/vtr_reg_strong_odin/strong_custom_grid/config/golden_results.txt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (14)
libs/librrgraph/src/base/rr_graph_builder.cpp (2)
libs/librrgraph/src/base/rr_graph_view.h (2)
edge_switch(475-477)edge_switch(480-482)libs/librrgraph/src/base/rr_graph_storage.h (1)
edge_switch(1360-1362)
libs/librrgraph/src/base/rr_graph_view.cpp (2)
libs/librrgraph/src/base/rr_graph_view.h (2)
edge_sink_node(496-498)edge_sink_node(505-507)libs/librrgraph/src/base/rr_graph_storage.h (1)
edge_sink_node(1347-1349)
vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/crr_edge_builder.cpp (3)
vpr/src/base/vpr_context.h (1)
rr_graph_builder(227-227)libs/libarchfpga/src/read_xml_arch_file.cpp (1)
VTR_ASSERT(112-115)libs/libvtrutil/src/vtr_sentinels.h (1)
INVALID(33-33)
vpr/src/route/rr_graph_generation/tileable_rr_graph/tileable_rr_graph_gsb.cpp (2)
vpr/src/base/vpr_context.h (1)
rr_graph_builder(227-227)vpr/src/route/rr_graph_generation/tileable_rr_graph/rr_gsb.cpp (16)
side_manager(53-53)side_manager(60-60)side_manager(69-69)side_manager(135-135)side_manager(149-149)side_manager(166-166)side_manager(181-181)side_manager(212-212)side_manager(239-239)side_manager(253-253)side_manager(260-260)side_manager(274-274)side_manager(281-281)side_manager(295-295)side_manager(303-303)side_manager(433-433)
vpr/src/place/delay_model/PlacementDelayModelCreator.cpp (1)
vpr/src/route/router_delay_profiling.cpp (2)
alloc_routing_structs(239-274)alloc_routing_structs(239-245)
vpr/src/base/stats.h (2)
vpr/src/base/stats.cpp (2)
write_sb_count_stats(220-323)write_sb_count_stats(220-222)vpr/src/base/netlist.h (1)
string(474-1074)
vpr/src/route/rr_graph_generation/tileable_rr_graph/tileable_rr_graph_builder.cpp (1)
vpr/src/base/vpr_context.h (1)
rr_graph_builder(227-227)
vpr/src/route/rr_graph_generation/rr_graph_switch_utils.cpp (2)
vpr/src/base/vpr_context.h (1)
rr_graph_builder(227-227)libs/librrgraph/src/base/rr_graph_view.h (1)
rr_switch_inf(719-721)
vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/data_frame_processor.h (1)
vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/crr_common.h (1)
crrgenerator(15-117)
vpr/src/route/rr_graph_generation/tileable_rr_graph/tileable_rr_graph_edge_builder.cpp (4)
libs/librrgraph/src/base/rr_graph_builder.cpp (2)
node_lookup(18-20)node_lookup(18-18)vpr/src/route/rr_graph_generation/tileable_rr_graph/tileable_rr_graph_gsb.cpp (6)
build_gsb_track_to_ipin_map(1506-1583)build_gsb_track_to_ipin_map(1506-1510)build_gsb_opin_to_track_map(1597-1680)build_gsb_opin_to_track_map(1597-1602)build_gsb_track_to_track_map(438-534)build_gsb_track_to_track_map(438-446)vpr/src/route/rr_graph_generation/build_switchblocks.cpp (1)
sb_conn(385-385)vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/crr_edge_builder.cpp (2)
build_crr_gsb_edges(62-85)build_crr_gsb_edges(62-65)
vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/node_lookup_manager.cpp (1)
vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/node_lookup_manager.h (1)
NodeLookupManager(28-88)
vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/node_lookup_manager.h (1)
vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/node_lookup_manager.cpp (17)
NodeLookupManager(11-14)initialize(16-33)initialize(16-16)get_column_nodes(35-38)get_column_nodes(35-35)get_row_nodes(40-43)get_row_nodes(40-40)get_combined_nodes(45-57)get_combined_nodes(45-45)print_statistics(59-89)print_statistics(59-59)clear(91-94)clear(91-91)build_node_hash(96-102)build_node_hash(96-96)index_node(104-131)index_node(104-104)
vpr/src/base/vpr_api.cpp (1)
vpr/src/base/stats.cpp (2)
write_sb_count_stats(220-323)write_sb_count_stats(220-222)
vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/data_frame_processor.cpp (1)
vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/data_frame_processor.h (3)
DataFrame(75-130)at(82-83)count_non_empty(102-113)
🔇 Additional comments (66)
vtr_flow/tasks/regression_tests/vtr_reg_strong/strong_xilinx_flagship/config/golden_results.txt (1)
3-3: Substantial regression test data changes require explicit justification and documentation.The updated row shows significant metric increases that substantially differ from the previous baseline:
- Device characteristics: num_primary_inputs (11 → 66), num_primary_outputs (2 → 96)
- Memory usage: max_vpr_mem (75864 → 245520, ~3.2× increase)
- Timing: pack_time and other stage times substantially increased
Without explicit documentation in the commit message or inline comments, these changes are difficult to validate as intentional. Golden results are critical for regression detection.
Please clarify:
- Did the device/circuit characteristics change legitimately, or is this a configuration issue?
- Is the 3.2× memory increase expected due to CRR infrastructure overhead, or does it indicate a potential regression?
- What validation confirms these new golden values are correct?
Consider adding a commit message note or comment block explaining the expected impact on regression metrics from CRR integration.
vtr_flow/tasks/regression_tests/vtr_reg_strong/strong_custom_grid/config/golden_results.txt (1)
8-8: Verify justification for ~50% memory increase in multiple_io_types.xml test.The max_vpr_mem field increased from 522,768 to 784,152 KiB for the multiple_io_types.xml test case. This represents a substantial 50% increase compared to the baseline. Given that the PR introduces CRR (Custom Routing Resources) with new data structures, YAML parsing, and pattern-based graph construction, the increase may be expected—but it warrants explicit verification.
Please confirm:
- What specific CRR-related data structures or allocations drove this 50% memory growth?
- Is this memory overhead proportional to circuit complexity, or is it a fixed cost regardless of circuit size?
- Was memory profiling performed to identify and justify each major allocation contributor?
- Is this memory increase acceptable for the new feature, or should optimizations be prioritized?
vtr_flow/tasks/regression_tests/vtr_reg_strong_odin/strong_custom_grid/config/golden_results.txt (1)
8-8: Verify the ~100% memory increase for themultiple_io_types.xmltest case is expected.The
max_vpr_memvalue has doubled from 483,652 to 967,304 for this test case. While this is likely an expected consequence of adding the CRR feature (which introduces additional data structures and RR graph modifications), this magnitude of increase warrants explicit confirmation.Please confirm:
- Whether this memory increase has been analyzed and is acceptable for the feature.
- Whether any optimization opportunities have been explored or are planned.
- Whether memory usage scales linearly with design complexity or if this is a fixed overhead.
vpr/src/route/rr_graph_generation/tileable_rr_graph/tileable_rr_graph_gsb.cpp (1)
1193-1235: Confirm inconsistency between regular and VIB edge-building functions.The observation is correct:
build_edges_for_one_tileable_rr_gsbincludes guards checkingmap.size() > 0for all three map types (opin2track_map, track2ipin_map, track2track_map), whilebuild_edges_for_one_tileable_rr_gsb_vibomits all three guards and directly iterates over the maps.This pattern suggests the difference is intentional—VIB likely operates under different preconditions and guarantees that maps are always populated, whereas the regular tileable RR graph builder may encounter empty maps depending on configuration (e.g., with CRR enabled). However, clarification on why VIB skips these defensive checks would help ensure this assumption is correct and prevent future bugs if preconditions change.
vpr/src/route/router_delay_profiling.cpp (1)
239-271: LGTM!The
crr_optsparameter is correctly threaded through tocreate_rr_graph. The propagation is consistent with the CRR infrastructure changes across the codebase.vpr/src/route/rr_graph_generation/tileable_rr_graph/tileable_rr_graph_edge_builder.h (2)
23-45: LGTM!The signature updates for
build_rr_graph_edgescorrectly addcrr_optsandroute_verbosityparameters to support CRR-aware edge building.
81-101: LGTM!The
build_rr_graph_regular_edgessignature is appropriately updated. The change from non-const to const reference forrr_node_driver_switchesis a good improvement, indicating read-only usage.libs/librrgraph/src/base/rr_graph_view.h (1)
66-66: LGTM!The addition of
rr_graph_fwd.hinclude is appropriate for supporting forward declarations needed by CRR-related components that depend on this header.libs/librrgraph/src/base/rr_graph_storage.cpp (1)
21-24: LGTM! Formatting improvement.The multi-line formatting of the function signature improves readability and is consistent with the corresponding declaration in the header file.
libs/librrgraph/src/base/rr_graph_builder.cpp (1)
179-182: LGTM! Formatting improvement.The multi-line formatting of the function signature improves readability and is consistent with similar formatting changes across related files.
vpr/src/base/stats.h (1)
26-28: LGTM! New CRR statistics function.The new function declaration for switchbox count statistics is well-placed and follows the existing naming conventions. The implementation is documented in the AI summary as part of the CRR feature integration.
libs/librrgraph/src/base/rr_graph_storage.h (2)
23-23: Good practice: explicit include.Adding the explicit
#include <string>is proper practice since the class usesstd::stringin several member declarations (e.g.,node_name_andvirtual_clock_network_root_idx_maps). This makes the dependency explicit rather than relying on transitive includes.
830-833: LGTM! Formatting improvement.The multi-line formatting of the function signature improves readability and maintains consistency with the implementation in rr_graph_storage.cpp.
utils/route_diag/src/main.cpp (1)
297-303: PassingCRROptsintoalloc_routing_structskeeps route_diag consistent with the main VPR flowThreading
vpr_setup.CRROptsthrough here is straightforward and matches the intent to share CRR configuration across tools. No issues from this call site..gitmodules (1)
19-21: yaml-cpp submodule wiring looks consistent with the build integrationThe submodule path and URL align with the CMake integration under
libs/EXTERNALand should work as expected. You may just want to confirm the submodule revision and target name match whatlibs/EXTERNAL/CMakeLists.txtassumes.doc/src/vpr/command_line_usage.rst (1)
1-2: Command-line usage anchor is correct and usefulThe
.. _vpr_command_line_usage:label is valid reST and will make cross-references to this section cleaner.libs/EXTERNAL/CMakeLists.txt (1)
16-23: yaml-cpp external integration matches existing system-header patternAdding
add_subdirectory(yaml-cpp)here and then marking itsINTERFACE_INCLUDE_DIRECTORIESasINTERFACE_SYSTEM_INCLUDE_DIRECTORIESis consistent with how other externals are handled to suppress warnings. Theif(TARGET yaml-cpp)guard is also reasonable. Just ensure the submodule actually defines ayaml-cppCMake target with interface include dirs set, so this stays effective.libs/librrgraph/src/io/rr_graph.xsd (1)
138-140: Optionaltemplate_idattribute onswitchis schema-safeAdding
template_idas an optionalxs:stringonswitchcleanly extends the schema and keeps existing rr_graph XML valid. This aligns with the new in-memory switch metadata and shouldn’t affect consumers that ignore the attribute.libs/libarchfpga/src/physical_types.h (1)
1762-1807: Switch-leveltemplate_idmetadata is a reasonable, non-intrusive extensionAdding
template_idtot_arch_switch_infas a default-initialisedstd::stringkeeps the struct backward-compatible and provides a clean hook for CRR analyses. From this header alone there’s no behavioural change; just ensure the corresponding arch/rr-graph (de)serialisation and any debug/printing paths are updated so this metadata is preserved and visible where needed.vpr/src/base/setup_vpr.h (1)
8-31: SetupVPR signature change is coherent but is a public API breakAdding
t_crr_opts* CRROptshere is consistent with the rest of the CRR plumbing and makes sense functionally. Note, however, that this is a source-level breaking change for any external code callingSetupVPR; if there are out-of-tree users depending on this header, they’ll need to be updated or provided with a compatibility shim/overload.vpr/src/base/place_and_route.h (1)
20-36: Threading CRR options through binary_search_place_and_route looks consistentThe added
const t_crr_opts& crr_optsfits the existing calling pattern (afterrouter_opts, before analysis/NOC/file options) and is safe as a const reference. Please just confirm that all definitions and callsites were updated to this new signature so you do not get mismatched-declaration link errors.vpr/src/route/rr_graph_generation/rr_graph_switch_utils.cpp (1)
90-121: template_id metadata is now correctly propagated from arch switches to RR switchesCopying
template_idfromt_arch_switch_infinto both the builder-backedrr_switch()[rr_switch_idx]and the standalonet_rr_switch_infreturned bycreate_rr_switch_from_arch_switchkeeps the metadata consistent with the rest of the switch fields and makes it available for downstream CRR analysis.No functional issues seen here.
Also applies to: 123-156
vpr/src/analytical_place/analytical_placement_flow.cpp (1)
235-248: Analytical placement now honours CRR options in delay-model creationForwarding
vpr_setup.CRROptsintoPlacementDelayModelCreator::create_delay_modelkeeps the analytical placement flow consistent with the main placement flow and ensures any CRR-specific RR-graph configuration is reflected in the delay model as well. This looks correct as long as the creator’s signature matches this ordering across all callsites.vpr/src/place/place.cpp (1)
28-40: CRR options are correctly threaded into try_place and its delay modelExtending
try_placewithconst t_crr_opts& crr_optsand forwarding it intoPlacementDelayModelCreator::create_delay_modelis consistent with the rest of the CRR plumbing and ensures timing-driven placement uses a delay model built under the same CRR configuration as routing.Please just ensure all callers of
try_placehave been updated to pass at_crr_optsinstance in this new position.Also applies to: 83-94
vpr/src/route/route_utils.h (1)
149-157: try_graph signature change for CRR options is consistent with the rest of the APIAdding
const t_crr_opts& crr_optsafterrouter_optsmatches the ordering used in other entry points and is an appropriate place for RR-graph–related configuration.Please confirm that:
- The implementation of
try_graphin the.cppfile takes the same parameter list and forwardscrr_optsintocreate_rr_graph, and- All callsites have been updated to the new signature.
libs/librrgraph/src/base/rr_graph_view.cpp (1)
62-71: find_edges now uses canonical edge IDs in a way consistent with validate_in_edgesSwitching
find_edgesto deriveRREdgeId edge = node_storage_.edge_id(src_node, iedge);and then usingedge_sink_node(edge)both matches the storage API and is consistent with the pattern used invalidate_in_edges. This keeps the method returning canonical edge IDs without changing its observable behaviour.libs/librrgraph/src/base/rr_graph_builder.h (3)
15-15: LGTM!The explicit
#include <string>ensures proper dependencies for thestd::stringparameter used inset_node_name()at line 140.
293-296: LGTM!The line wrapping improves readability without any semantic changes to the function signatures.
Also applies to: 339-342
411-419: LGTM! Clear documentation of OpenFPGA-specific usage.The
unlock_storage()method correctly resets the internal state flags and clears the first edge storage. The documentation clearly indicates this is for OpenFPGA use and enables post-edge-read modifications.vpr/src/route/rr_graph_generation/tileable_rr_graph/tileable_rr_graph_builder.h (1)
16-38: LGTM! API extension for CRR support.The function signature has been properly extended to accept CRR options (
crr_optsat line 19) and routing verbosity (route_verbosityat line 37). This enables the tileable RR graph builder to support custom routing resource generation.vpr/src/base/vpr_api.h (1)
173-196: LGTM! Public API extension for CRR options.The addition of
t_crr_opts* CRROptsparameter at line 184 properly extends the VPR setup API to support Custom RR Graph generation options, consistent with the existing parameter pattern.vpr/src/route/router_delay_profiling.h (1)
47-53: LGTM! Routing structure allocation extended for CRR support.The addition of
const t_crr_opts& crr_optsparameter at line 49 properly extendsalloc_routing_structsto support CRR options during routing structure allocation.vpr/src/route/rr_graph_generation/rr_graph.h (1)
25-35: LGTM! Core RR graph API extended for CRR support.The addition of
const t_crr_opts& crr_optsparameter at line 32 properly extends the main RR graph creation function to support Custom RR Graph generation. The parameter placement is logical and consistent with the overall API design.vpr/src/place/delay_model/PlacementDelayModelCreator.h (2)
11-11: LGTM! Proper forward declaration.The forward declaration of
struct t_crr_optsis appropriately added to avoid including the full header.
22-30: LGTM! Delay model creation extended for CRR support.The addition of
const t_crr_opts& crr_optsparameter at line 24 properly extends the delay model creation to support CRR options, enabling CRR-aware delay modelling during placement.vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/crr_edge_builder.h (1)
1-26: LGTM! Past review comments addressed.The header file is well-structured with:
- Proper file documentation (lines 3-8) addressing the file comment request
- Complete function documentation (lines 16-22) addressing the docstring request
- All four includes are necessary and used in the function signature
The
build_crr_gsb_edgesfunction provides a clean interface for CRR edge construction with well-documented parameters.vpr/src/route/route_utils.cpp (1)
493-533: Threadingt_crr_optsthroughtry_graphlooks consistentAdding
const t_crr_opts& crr_optstotry_graphand forwarding it tocreate_rr_graphpreserves existing behaviour while enabling CRR configuration; parameter ordering is consistent with the rest of the routing API. No issues spotted here.vpr/src/route/route.h (1)
20-33: Route API extension witht_crr_optsis reasonableExtending
route()withconst t_crr_opts& crr_optsdirectly afterrouter_optskeeps the API coherent (all routing options grouped) and matches usage elsewhere. Just ensure all callers and any external users of this header have been updated accordingly.If you want an automated check for any stale declarations, you can run a simple search over the tree for other
route(declarations and definitions and confirm they includet_crr_opts.vpr/src/place/delay_model/PlacementDelayModelCreator.cpp (1)
26-47: CRR options correctly wired into placement delay-model creationPassing
const t_crr_opts& crr_optsintoPlacementDelayModelCreator::create_delay_modeland forwarding it toalloc_routing_structsaligns the delay-model RR-graph construction with the CRR configuration used by routing. The argument order matches the updatedalloc_routing_structssignature; this looks sound.libs/librrgraph/src/io/rr_graph_uxsdcxx_serializer.h (1)
519-555:template_idaccessors for switches are minimal and consistent
set_switch_template_id/get_switch_template_idcorrectly map the XMLtemplate_idattribute to the newt_rr_switch_inf::template_idstring field, mirroring the existing name accessors and avoiding extra ownership complexity. This should integrate cleanly with the updated schema and switch structures.vpr/src/route/route.cpp (1)
17-30: CRR options threading intoroute()andcreate_rr_graph()looks consistentThe added
const t_crr_opts& crr_optsparameter is cleanly plumbed through tocreate_rr_graphwithout altering routing logic, which is what we want for CRR integration at this layer.Also applies to: 59-69
vpr/src/analytical_place/detailed_placer.cpp (1)
48-74: CRR options plumbed into detailed placer delay model correctlyPassing
vpr_setup.CRROptsintoPlacementDelayModelCreator::create_delay_modelhere keeps the detailed placer consistent with the rest of the CRR plumbing, without changing annealing logic. The constructor remains straightforward and the new argument is positioned logically alongside router options.vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/crr_connection_builder.h (3)
1-7: LGTM on file documentation.The file comment clearly describes the purpose of this header. This addresses a previous review suggestion.
58-61: Potential thread-safety concern with atomic member in const context.
processed_locations_is anstd::atomicthat appears to be modified byupdate_progress(), butget_tile_connections()is markedconst. If this method callsupdate_progress()through a non-const path, theconstqualifier may be misleading. Consider whether this design is intentional (e.g., progress tracking is logically const) or if the method signature should be revisited.
23-46: Public API structure is well-defined.The constructor accepts the required dependencies (RRGraphView, NodeLookupManager, SwitchBlockManager), and the public interface is clean with
initialize()andget_tile_connections(). The separation of initialization from construction allows for flexible usage patterns.vpr/src/base/place_and_route.cpp (4)
41-57: LGTM: CRR options parameter threading.The
crr_optsparameter is correctly added to the function signature and consistently passed through to all relevant call sites (try_place,route,create_rr_graph). The placement afterrouter_optsmaintains logical parameter grouping.
173-198: LGTM: CRR options passed to placement and routing calls.The
crr_optsparameter is correctly threaded through totry_placeandroutewithin the main binary search loop.
318-340: LGTM: Verification loop correctly uses CRR options.The verify_binary_search loop correctly passes
crr_optsto bothtry_placeandroutecalls, maintaining consistency with the main search loop.
374-384: LGTM: Final RR graph creation receives CRR options.The
crr_optsparameter is correctly passed tocreate_rr_graphfor the final graph construction after binary search completes.vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/node_lookup_manager.cpp (1)
104-131: LGTM: Node indexing logic is correct.The
index_nodemethod correctly skips SOURCE/SINK nodes and indexes single-column/single-row nodes appropriately. The boundary assertions ensure valid grid coordinates.vpr/src/place/place.h (1)
8-19: LGTM: Function signature update for CRR options.The
crr_optsparameter is correctly added to thetry_placedeclaration, maintaining consistent ordering with the implementation inplace_and_route.cpp.vpr/src/base/vpr_types.h (1)
1630-1630: LGTM: CRR options integrated into t_vpr_setup.The
CRROptsmember is appropriately placed alongside other option structs int_vpr_setup, maintaining consistency with the existing pattern.vpr/src/route/rr_graph_generation/tileable_rr_graph/tileable_rr_graph_builder.cpp (2)
335-380: Solid implementation of dangling node removal.The logic correctly:
- Identifies dangling CHANX/CHANY nodes with zero fan-in
- Removes them from the graph
- Rebuilds the node lookup structures
- Reinitializes fan-in counts
One consideration: the loop at lines 365-375 handles both multi-ptc and single-loc nodes differently, which is appropriate for maintaining lookup consistency.
81-103: LGTM on signature extension.The new
crr_optsandroute_verbosityparameters are correctly added to the function signature and properly threaded through tobuild_rr_graph_edges.vpr/src/route/rr_graph_generation/tileable_rr_graph/tileable_rr_graph_edge_builder.cpp (2)
330-343: CRR component initialization looks correct.The initialization sequence properly:
- Initializes
SwitchBlockManagerwith maps and templates- Initializes
NodeLookupManagerfor node resolution- Creates
CRRConnectionBuilderwith dependenciesThe use of
std::make_uniquefor the connection builder is appropriate since it's conditionally created.
371-388: Path switching between CRR and legacy flows is well-structured.The conditional logic cleanly separates CRR-enabled flow from the legacy switch-block connection building, while ensuring boundary GSBs are excluded from CRR processing (
ix != gsb_range.x() && iy != gsb_range.y()).vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/crr_common.h (2)
71-77: Inconsistent use ofsw_template_id_in comparison operators.Both
operator<andoperator==excludesw_template_id_from comparison. If this is intentional (connections with different template IDs but same nodes/delay are considered equal), please add a brief comment clarifying this design decision.
96-115: Well-implemented hash function.The
NodeHasheruses the golden ratio constant (0x9e3779b9) with bit mixing, which is a proven technique for good hash distribution. The implementation correctly combines all six tuple components.vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/crr_edge_builder.cpp (1)
62-85: Edge building logic is correct.The function properly:
- Retrieves connections for the GSB tile
- Handles the special case of
delay_ps == -1(use architecture-defined switch)- Asserts switch validity before creating edges
- Creates edges with correct source/sink ordering
vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/crr_switch_block_manager.h (2)
82-119: Well-documented private members.The private section has clear documentation for each member, explaining the purpose of
ordered_switch_block_patterns_for deterministic pattern resolution and the caching strategy. The YAML validation helper is appropriately private.
26-80: Clean public API design.The class provides a well-designed interface with:
- Clear initialization workflow (
initialize)- Query methods for pattern lookup and validation
- Statistics/debugging support (
print_statistics,get_total_connections)- Pattern-first matching semantics documented for
find_matching_patternvpr/src/base/read_options.h (1)
287-296: LGTM!The CRR options are well-organized and follow the existing pattern used throughout the file. The types are appropriate for each option (strings for file paths, int for thread count, booleans for flags).
vpr/src/base/vpr_api.cpp (2)
300-301: LGTM!CRROpts is properly threaded through the SetupVPR call, consistent with other option structures.
1512-1516: LGTM!The conditional write of SB count statistics follows a clean pattern, only executing when
sb_count_diris non-empty.vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/crr_connection_builder.cpp (1)
15-25: LGTM!Good documentation explaining the
from_charsparsing logic and return conditions.vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/node_lookup_manager.h (1)
1-90: LGTM!The file is well-documented with clear doxygen comments. The explicit
@noteabout eventual removal and integration with the existing RR graph lookup is good documentation of technical debt.
vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/crr_common.h
Show resolved
Hide resolved
vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/crr_edge_builder.cpp
Outdated
Show resolved
Hide resolved
vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/data_frame_processor.cpp
Show resolved
Hide resolved
vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/data_frame_processor.cpp
Show resolved
Hide resolved
vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/data_frame_processor.cpp
Show resolved
Hide resolved
vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/data_frame_processor.cpp
Show resolved
Hide resolved
vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/node_lookup_manager.cpp
Show resolved
Hide resolved
AmirhosseinPoolad
left a comment
AmirhosseinPoolad
left a comment
There was a problem hiding this comment.
Thanks for the PR Amin! Had some thoughts, looks good overall.
| * @brief Unlock storage; required to modify an routing resource graph after edge is read | ||
| * @note This function is used by OpenFPGA and currently doesn't have any use in VPR code. | ||
| */ | ||
| inline void unlock_storage() { |
There was a problem hiding this comment.
I think you can merge this with RRGraphBuilder::reset_rr_graph_flags. As in, same name, same functionality, reset_rr_graph_flags calls this.
vpr/src/route/rr_graph_generation/tileable_rr_graph/crr_generator/node_lookup_manager.h
Show resolved
Hide resolved
vpr/src/route/rr_graph_generation/tileable_rr_graph/tileable_rr_graph_builder.cpp
Show resolved
Hide resolved
vtr_flow/tasks/regression_tests/vtr_reg_strong/strong_xilinx_flagship/config/golden_results.txt
Outdated
Show resolved
Hide resolved
vtr_flow/tasks/regression_tests/vtr_reg_strong/strong_custom_grid/config/golden_results.txt
Outdated
Show resolved
Hide resolved
...flow/tasks/regression_tests/vtr_reg_strong_odin/strong_custom_grid/config/golden_results.txt
Outdated
Show resolved
Hide resolved
… to higher than 1
In this PR, a new custom RR Graph generator for tileable RR Graphs is introduced. With this generator, users can define their desired switch block patterns in CSV files (one CSV file per unique pattern) and specify, in a YAML switch-block map file, which pattern should be applied at each tile location. When generating the RR Graph, instead of relying on the switch block definitions in the architecture file, the CRR generator reads these CSV templates and constructs the connections accordingly.
This work was done on top of the work that was started by @ganeshgore, @ql-mahdi, and @saaramahmoudi.
Summary by CodeRabbit
New Features
--sb_maps,--sb_templates,--sb_count_dir, etc.).Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.