Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up rr_node_route_inf #2499

Closed
vaughnbetz opened this issue Mar 13, 2024 · 4 comments
Closed

Clean up rr_node_route_inf #2499

vaughnbetz opened this issue Mar 13, 2024 · 4 comments
Assignees

Comments

@vaughnbetz
Copy link
Contributor

vaughnbetz commented Mar 13, 2024

  1. We have a dead member variable (target_flag) in rr_node_route_inf (not used anymore since we deleted the breadth-first router). We should delete it.
  2. The comment for the prev_edge says it is the index within the switch list of the predecessor node used to reach the current node. I'm pretty sure that is not correct anymore, since we now have globally unique edge ids (do we use that large id instead)? We should check and correct the comment if needed.
  3. If we use globally unique edge ids we could use that to find the predecessor node, and not store it anymore. This should be investigated to see if it saves memory / speeds up the router (or if it slows it down, don't do it).

/**
 * @brief Extra information about each rr_node needed only during routing
 *        (i.e. during the maze expansion).
 *
 *   @param prev_node  Index of the previous node (on the lowest cost path known
 *                     to reach this node); used to generate the traceback.
 *                     If there is no predecessor, prev_node = NO_PREVIOUS.
 *   @param prev_edge  Index of the edge (from 0 to num_edges-1 of prev_node)
 *                     that was used to reach this node from the previous node.
 *                     If there is no predecessor, prev_edge = NO_PREVIOUS.
 *   @param acc_cost   Accumulated cost term from previous Pathfinder iterations.
 *   @param path_cost  Total cost of the path up to and including this node +
 *                     the expected cost to the target if the timing_driven router
 *                     is being used.
 *   @param backward_path_cost  Total cost of the path up to and including this
 *                     node.
 *   @param target_flag  Is this node a target (sink) for the current routing?
 *                     Number of times this node must be reached to fully route.
 *   @param occ        The current occupancy of the associated rr node
 */
struct t_rr_node_route_inf {
    RRNodeId prev_node;
    RREdgeId prev_edge;

    float acc_cost;
    float path_cost;
    float backward_path_cost;

    short target_flag;

  public: //Accessors
    short occ() const { return occ_; }

  public: //Mutators
    void set_occ(int new_occ) { occ_ = new_occ; }

  private: //Data
    short occ_ = 0;
};



AlexandreSinger added a commit to AlexandreSinger/vtr-verilog-to-routing that referenced this issue Mar 13, 2024
The target_flag member used to be used by a different router; however,
that router has since been removed. This data was still being collected
and stored, but was going unused. Removed the member which would save
space and should improve performance slightly.

See issue verilog-to-routing#2499
AlexandreSinger added a commit to AlexandreSinger/vtr-verilog-to-routing that referenced this issue Mar 16, 2024
The target_flag member used to be used by a different router; however,
that router has since been removed. This data was still being collected
and stored, but was going unused. Removed the member which would save
space and should improve performance slightly.

See issue verilog-to-routing#2499
@AlexandreSinger
Copy link
Contributor

The acc_cost and occ_ members appear to not be used in the maze expansion (against what the comment above this class is saying). We should separate out the per-iteration route info from the maze-expansion route info. Beyond organization, this would reduce the amount of data being stored in the cache during maze expansion and also reduce the number of global variables the maze expansion touches.

This would require creating a new struct to hold this "per-iteration" routing info and would require modifying the RoutingContext to also hold this information:

struct RoutingContext : public Context {
    // ...

    vtr::vector<RRNodeId, t_rr_node_route_inf> rr_node_route_inf; /* [0..device_ctx.num_rr_nodes-1] */

    // ...
};

Perhaps the new type could be named rr_node_per_iter_route_inf, but I am concerned the name might be too long.

@vaughnbetz What do you think about this change?

@amin1377
Copy link
Contributor

@AlexandreSinger As discussed: The occ_ field is read in the connection router (maze expansion) to calculate the congestion cost (Link)

@vaughnbetz
Copy link
Contributor Author

acc_cost is also read during the connection router (maze expansion); it just isn't written during maze expansion.

@AlexandreSinger
Copy link
Contributor

The PR resolving this issue has been merged into master. Closing this issue. Since the occ and acc_cost are read during the wave expansion, it should be fine to keep them in this struct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants