Noc traffic flows parser final #2083
Conversation
…nclude a Network on chip
…ute and modified some function prototypes
…rd topologies such as 'mesh'
…architectyre file
… input. Also added a function that isolates noc router tiles from the device grid.
…d the object definition for a noc_link.
… router ids to the id system used for routers in the NoC
… in the arch description file
…e class that pre-allocates space for datastructures
…on the FPGA device
…m found the closest physical router to each logical router, instead of the other way around.
…sitions calculated. Also added unit tests to very the change.
…ween router ids and NocRouter ids
…e added accidently before
vpr/src/base/clustered_netlist.cpp
Outdated
| @@ -274,6 +276,28 @@ void ClusteredNetlist::shrink_to_fit_impl() { | |||
| net_is_global_.shrink_to_fit(); | |||
| } | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
Remove this data since it is only used in one parser and instead create locally (in a map or whatever you want there). Easier to understand and maintain since the data is local and has a short lifetime then.
Right now you're adding global state that all netlist modifiers (delete_block for example) will have to keep up to date. Since it's only used in one spot, it isn't worth it.
vpr/src/base/clustered_netlist.cpp
Outdated
| * additional datastructure is created that groups clusters by their | ||
| * logical type. This function filters the clusters and only searches | ||
| * for the matching block within a list of blocks that are the same | ||
| * logical type. The idea here is that the filtered list should be |
There was a problem hiding this comment.
Can just make this routine take a pass in vector of blocks to search if you want to keep it efficient (can compute that vector once in the calling routine); if someone didn't know the vector of blocks they could pass in a vector of the whole netlist
There was a problem hiding this comment.
Removed this new data-structure from the clustered netlist,
vpr/src/base/clustered_netlist.cpp
Outdated
| @@ -306,3 +330,52 @@ bool ClusteredNetlist::validate_net_sizes_impl(size_t num_nets) const { | |||
| } | |||
| return true; | |||
| } | |||
|
|
|||
| /** | |||
| * @brief Finds a block where the blocks name contains within it the | |||
There was a problem hiding this comment.
Will have to update comment .
Typo: block's name
vpr/src/base/clustered_netlist.h
Outdated
| @@ -242,6 +250,9 @@ class ClusteredNetlist : public Netlist<ClusterBlockId, ClusterPortId, ClusterPi | |||
| ///@brief Shrinks internal data structures to required size to reduce memory consumption | |||
| void shrink_to_fit_impl() override; | |||
|
|
|||
| ///@brief Group the block to its corresponding logical type | |||
| void add_block_to_logical_type(ClusterBlockId blk_id, t_logical_block_type_ptr type); | |||
There was a problem hiding this comment.
Still in .h file --> please remove. Also check for any other dangling definitions in .h files.
There was a problem hiding this comment.
Removed and went through all other header files, did not see any other dangling function definitions.
vpr/src/base/clustered_netlist.h
Outdated
| @@ -277,8 +288,10 @@ class ClusteredNetlist : public Netlist<ClusterBlockId, ClusterPortId, ClusterPi | |||
| vtr::vector_map<ClusterBlockId, t_pb*> block_pbs_; ///<Physical block representing the clustering & internal hierarchy of each CLB | |||
| vtr::vector_map<ClusterBlockId, t_logical_block_type_ptr> block_types_; ///<The type of logical block this user circuit block is mapped to | |||
| vtr::vector_map<ClusterBlockId, std::vector<ClusterPinId>> block_logical_pins_; ///<The logical pin associated with each physical tile pin | |||
|
|
|||
| //Pins | |||
| std::unordered_map<std::string, std::vector<ClusterBlockId>> block_type_to_id; ///<Group the physical blocks by their logical types | |||
vpr/src/base/netlist.h
Outdated
| @@ -727,6 +727,13 @@ class Netlist { | |||
| */ | |||
| BlockId find_block(const std::string& name) const; | |||
|
|
|||
| /** | |||
| * @brief Returns the BlockId of the specified block or BlockId::INVALID() if not found. The name of the block returned contains the provided input name in it. | |||
There was a problem hiding this comment.
Change to find_block_by_name_substring (or by_name_fragment). Make that clear in comment too.
There was a problem hiding this comment.
Say it returns first match if there are multiple.
There was a problem hiding this comment.
Also can move some or all of the .cpp example comment here.
There was a problem hiding this comment.
Updated function name and also incorporated these changes into the comments
vpr/src/noc/noc_traffic_flows.h
Outdated
| * used before each iteraition of placement. | ||
| * | ||
| */ | ||
| void reset_traffic_flows_processed_status(void); |
There was a problem hiding this comment.
Think not needed after refactoring.
vpr/src/noc/noc_traffic_flows.h
Outdated
| * @return true The block is a router | ||
| * @return false THe block is not a router | ||
| */ | ||
| bool check_if_cluster_block_is_a_noc_router(ClusterBlockId block_id); |
There was a problem hiding this comment.
maybe replace with has_traffic_flows (or just comment that it checks by seeing if you have traffic flows)
--> is_noc_router_with_flow
There was a problem hiding this comment.
Changed to check_if_cluster_block_has_traffic_flows
| * same names. THe two routers cant be the exact same since a router | ||
| * cannot communicate with itself. | ||
| * | ||
| * @param source_router_name A string value of the source router module name |
There was a problem hiding this comment.
Mention can be partial names.
| * @param cluster_ctx Global variable that contains clustering information. | ||
| * Contains a datastructure to convert a module name to | ||
| * a cluster block id. | ||
| * @param single_flow_tag A xml tag that contains the traffic flow information |
There was a problem hiding this comment.
mention which are just for error messages.
| * @param noc_traffic_flow_storage Used to get the previously added traffic | ||
| * traffic flow information. | ||
| */ | ||
| void check_for_duplicate_traffic_flow(ClusterBlockId source_router_id, ClusterBlockId sink_router_id, pugi::xml_node single_flow_tag, const pugiutil::loc_data& loc_data, const NocTrafficFlows& noc_traffic_flow_storage); |
There was a problem hiding this comment.
Don't think this is really an error.
…t sorted clusteres by their logical block type. This removal then requried the 'find_block_with_matching_name' function to be modified as it used the deleted datastructure. Now this function accepts a vector of clusters that it searches through, instead of the entire netlist. Also updated the unit test for the modified function.
… all cluster blocks that are compatible with physical noc router tiles. This was done due to the modifactions made the 'find_block_with_matching_name' function in the previous commit. Added a corresponding unit test for this function. Also modified other functios in read_xml_noc_traffic_flows_file.cpp to bring in the changes made made to 'find_block_with_matching_name'
…ame_fragment'. Also improved commenting for this function. Updated all calls to this function as well.
…y, it was assumed that two traffic flows where the source and estination routers were the same was illegal. But it is was later found that this is not illegal and completely allowed.
… flows for each router in a single datastructure. Previouslythe traffic flow association was done using two seperate datastructures, where one was used to store traffic flows where the router was the starting point and the other was used to store traffic flows where the routers was the destination. Now they are grouped into one.
…ack of which traffic flows had been processed or routed already. This was originally inteded to be used during placement, where ine ach iteration traffic flows are re-routed. THis datastructure can be used to ensure that a traffic flow is no re-routed multiple times.THe decision was made to create a local variable during placement to keep track of this.
…ows class has all traffic flows added or not. Added the update of this variable within the 'finished_noc_traffic_flows_setup' function. The purpose of this is to ensure that traffic flows are only added once.
…ck of unique router cluster blocks within the netlist. The original intended use was to leverage this datastructure when a cluster is moved during placement to check whether the cluster was a router or not. Later, it was determined that this wasn't needed since another datastrurcture that was used to keep track of traffic flows associated to router cluster blocks could be used to determine whether a cluster has traffic flows or not, and if it did then we know its a router block.
…TrafficFlows class within the NoC Context
| // device information | ||
| const DeviceContext& device_ctx = g_vpr_ctx.device(); | ||
|
|
||
| // get the physical type of a noc router |
There was a problem hiding this comment.
These are common enough usage (one liners, with meaningful function names) that you don't have to comment them if you don't want to. OK to leave if you like though.
If you keep them, probably say you'll update the NoC.
| * compatible to physical NoC router tiles. | ||
| */ | ||
| std::vector<ClusterBlockId> cluster_blocks_compatible_with_noc_router_tiles; | ||
| get_cluster_blocks_compatible_with_noc_router_tiles(cluster_ctx, noc_router_tile_type, cluster_blocks_compatible_with_noc_router_tiles); |
There was a problem hiding this comment.
When returning one thing like this, clearer to just return the value (create and return) rather than taking a reference to an empty input and updating it.
| std::vector<ClusterBlockId> cluster_blocks_compatible_with_noc_router_tiles; | ||
| get_cluster_blocks_compatible_with_noc_router_tiles(cluster_ctx, noc_router_tile_type, cluster_blocks_compatible_with_noc_router_tiles); | ||
|
|
||
| /* variabled used when parsing the file */ |
There was a problem hiding this comment.
variables? Also comment what they do (current comment doesn't add much value).
| // check that only the accepted single flow attributes are found in the tag | ||
| pugiutil::expect_only_attributes(single_flow_tag, expected_single_flow_attributes, loc_data); | ||
|
|
||
| // store the names of the routers part of this traffic flow |
There was a problem hiding this comment.
Might want to add: should be a regex matching one logical router in the netlist.
|
|
||
| std::string sink_router_module_name = pugiutil::get_attribute(single_flow_tag, "dst", loc_data, pugiutil::REQUIRED).as_string(); | ||
|
|
||
| //verify whether the router module names are legal |
There was a problem hiding this comment.
(valid non-empty string)
| void verify_traffic_flow_router_modules(std::string source_router_name, std::string sink_router_name, pugi::xml_node single_flow_tag, const pugiutil::loc_data& loc_data) { | ||
| // check that the router module names were legal | ||
| if ((source_router_name == "") || (sink_router_name == "")) { | ||
| vpr_throw(VPR_ERROR_OTHER, loc_data.filename_c_str(), loc_data.line(single_flow_tag), "Invalid names for the source and sink NoC router modules."); |
There was a problem hiding this comment.
I'd split this into two tests and error messages, and say empty string.
| t_logical_block_type_ptr router_module_logical_type = cluster_ctx.clb_nlist.block_type(router_module_id); | ||
|
|
||
| /* | ||
| * Check whether the current router modules logical type is compatible |
There was a problem hiding this comment.
modules --> module's or module (either would work)
| } | ||
|
|
||
| t_physical_tile_type_ptr get_physical_type_of_noc_router_tile(const DeviceContext& device_ctx, NocContext& noc_ctx) { | ||
| // get a reference to a single physical noc router |
There was a problem hiding this comment.
Might mention you assume all routers have the same physical type, so you just return the type of the first one.
If there's any chance that there aren't any routers in the grid then add an assert or error message if physical_noc_router == .end() (should be the test for no router found).
| } | ||
|
|
||
| /* | ||
| * Every router block in the design needs to be part of a traffic flow. There can never be a router that isnt part of a traffic flow, other wise the router is doing nothing. So check that the number of unique routers in all traffic flows equals the number of router blocks in the design, otherwise throw an warning to let the user know. If there aren't |
There was a problem hiding this comment.
Long line; probably break.
isnt -> isn't
other wise -> otherwise
an warning -> a warning
vpr/src/base/clustered_netlist.cpp
Outdated
| * and then some other arbritary characters after. This pattern will | ||
| * then be used to match to the block in the netlist. | ||
| * | ||
| * This function runs in linear time (O(N)) as it goes over all the |
There was a problem hiding this comment.
Update this to say you go through the cluster_block_candidates, and are O(N) in that. The cluster_block_candidates could be the whole netlist, or a subset of the correct type, or anything else.
There was a problem hiding this comment.
(Unless this is redudant with the .h, in which case just shorten/ensure it's accurate).
vpr/src/base/clustered_netlist.cpp
Outdated
| * blocks that match to the provided input pattern, then the | ||
| * first block found is returned. | ||
| * | ||
| * There is a similiar function in the Netlist Class. This function |
There was a problem hiding this comment.
Delete this paragraph. No data structure stored anymore.
vpr/src/base/clustered_netlist.h
Outdated
| @@ -242,6 +250,9 @@ class ClusteredNetlist : public Netlist<ClusterBlockId, ClusterPortId, ClusterPi | |||
| ///@brief Shrinks internal data structures to required size to reduce memory consumption | |||
| void shrink_to_fit_impl() override; | |||
|
|
|||
| ///@brief Group the block to its corresponding logical type | |||
| void add_block_to_logical_type(ClusterBlockId blk_id, t_logical_block_type_ptr type); | |||
There was a problem hiding this comment.
Still in .h file --> please remove. Also check for any other dangling definitions in .h files.
vpr/src/base/vpr_types.h
Outdated
| @@ -1271,7 +1271,8 @@ struct t_analysis_opts { | |||
|
|
|||
| // used to store NoC specific options, when supplied as an input by the user | |||
| struct t_noc_opts { | |||
| bool noc; ///<options to model the noc within the FPGA device | |||
| bool noc; ///<options to model the noc within the FPGA device | |||
There was a problem hiding this comment.
Turns on hard NoC modeling & optimization
(update text in comment).
vpr/src/noc/noc_traffic_flows.cpp
Outdated
| #include "vpr_error.h" | ||
|
|
||
| // constructor indicates that the class has not been constructed yet | ||
| NocTrafficFlows::NocTrafficFlows(void) { |
There was a problem hiding this comment.
remove void.
Update comment: default constructor indicates the class hasn't been properly initiatilized yet.
vpr/src/noc/noc_traffic_flows.cpp
Outdated
|
|
||
| // check if there are any traffic flows associated with the current router | ||
| if (associated_traffic_flows != traffic_flows_associated_to_router_blocks.end()) { | ||
| // if we are here then there exists atleast 1 traffic flow that includes the current router as a source or sink |
vpr/src/noc/noc_traffic_flows.cpp
Outdated
| bool NocTrafficFlows::check_if_cluster_block_has_traffic_flows(ClusterBlockId block_id) { | ||
| auto traffic_flows = get_traffic_flows_associated_to_router_block(block_id); | ||
|
|
||
| bool result = false; |
There was a problem hiding this comment.
Could just say
if (traffic_flow == nullptr)
return false;
else
return true;
Better yet:
return (traffic_flows != nullptr);
…nts and updated README description for the NoC related benchmarks.
…in the NocTrafficFlows class to be simpler
… module namesof a traffic flow were invalid into two seperate errors which reported an error when either the src or destination router module names were invalid
… was done since a function in read_xml_noc_traffic_flows_file.cpp requires a single physical NoC router to determine its physical type. By having this assertion, we can prevent an invalid memory reads
…ion to return the vector of founds cluster blocks ids instead of modifying a reference vector that was originally passed in as a parameter
…ent function in the both the clustered netlist and netlist calsses to help indicate how the string parameter will be used when trying to find a block within the netlist.
…ine option in vpr
…ile to be more succint and also updated the echo file format
Added a NoC traffic flows file parser to be able to read in and store traffic flow information from the user. These traffic flows describe how routers are communicating within the NoC and this will assist the placement tool to determine whether a given placement of NoC routers is optimal or not.
Description
libvpr
Related Issue
Motivation and Context
During placement, to optimize the placement of the NoC routers we need information about how routers are communicating with each other. Some example things we should know are which routers are communicating with each other, what is the transmitted data size, are there any constraints on how fast the data should arrive at the destination. Using the previous information, we can tell whether a given placement of routers in the NoC is optimal or not. We can check whether any of the links between the two routers are overused, we can also determine whether the data will be received at the destination within the given constraints. The important note here is that additional information is needed about the NoC. This PR allows designers to provide this additional information to VPR.
How Has This Been Tested?
Types of changes
Checklist: