-
Notifications
You must be signed in to change notification settings - Fork 162
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
Fix tileable rr graph read/write issue. #680
Conversation
Yitian4Debug
commented
Jun 6, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@taoli4rs Need to generalize the code changes which may cause problems for many other architectures. We should not limit users when architecting FPGAs.
Also, we miss regression tests to validate the correctness of this feature.
My suggestion is to create a new test case where
- In openfpga shell, you call vpr's analysis only flow. You run verilog-to-verification flow as other test cases:
if you want to define input files for VPR, please consider to use the variables like
openfpga_vpr_device_layout=--device 2x2 --route_chan_width 40 |
OpenFPGA/openfpga_flow/openfpga_shell_scripts/write_full_testbench_example_script.openfpga
Line 3 in 70e2330
vpr ${VPR_ARCH_FILE} ${VPR_TESTBENCH_BLIF} --clock_modeling route ${OPENFPGA_VPR_DEVICE_LAYOUT} |
vpr/src/base/read_route.cpp
Outdated
@@ -47,6 +47,7 @@ static void process_nodes(std::ifstream& fp, ClusterNetId inet, const char* file | |||
static void process_nets(std::ifstream& fp, ClusterNetId inet, std::string name, std::vector<std::string> input_tokens, const char* filename, int& lineno); | |||
static void process_global_blocks(std::ifstream& fp, ClusterNetId inet, const char* filename, int& lineno); | |||
static void format_coordinates(int& x, int& y, std::string coord, ClusterNetId net, const char* filename, const int lineno); | |||
static void format_ptc_num(int& n0, int& n1, int& n2, int& n3, std::string coord, ClusterNetId net, const char* filename, const int lineno); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be careful. This is a corner case. You see 4 ptc numbers because of the use of Length-4 wires. Users are free to define any length of wires as supported by tileable rr_graph.
So I suggest to use a vector.
vpr/src/base/read_route.cpp
Outdated
if (device_ctx.rr_graph.node_ptc_num(node) != ptc) { | ||
int ptc0, ptc1, ptc2, ptc3; | ||
format_ptc_num(ptc0, ptc1, ptc2, ptc3, tokens[5 + offset], inet, filename, lineno); | ||
if (device_ctx.rr_graph.node_ptc_num(node) != ptc0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need a double check on the selection of ptc number. I forgot if we should always use the first one or the last one. For nodes whose direction is INC_DIR, I think we should use the first one. However, for the nodes whose direction is DEC_DIR, please double check if this is true or not.
vpr/src/base/read_route.cpp
Outdated
size_t ptc_count = std::count(ptc_str.begin(), ptc_str.end(), ',') + 1; | ||
// detect and remove the parenthesis | ||
std::stringstream ptc_stream(ptc_count > 1? format_name(ptc_str) : ptc_str); | ||
for (size_t i = 0; i < ptc_count; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May consider to use the tokenizer class:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@taoli4rs Please update regression tests in follow PRs.
Merge it now.