Make sure net is valid before setting it as wire LUT output

[chungshien]

Motivate of the pull request

• To address an existing issue. If so, please provide a link to the issue:
• Breaking new feature. If so, please describe details in the description part.

Describe the technical details

What is currently done? (Provide issue link if applicable)

In order to set and read if an output LUT is "wire LUT output", there are two functions that involved:

• set_wire_lut_output() - https://github.com/lnis-uofu/OpenFPGA/blob/master/openfpga/src/repack/physical_pb.cpp#L226
• is_wire_lut_output() - https://github.com/lnis-uofu/OpenFPGA/blob/master/openfpga/src/repack/physical_pb.cpp#L83

Function is_wire_lut_output() is called twice in the code

• https://github.com/lnis-uofu/OpenFPGA/blob/master/openfpga/src/repack/build_physical_truth_table.cpp#L128-L133 - From the code snippets, if the output net is invalid, the flow will be skipped for this output net
• https://github.com/lnis-uofu/OpenFPGA/blob/master/openfpga/src/utils/physical_pb_utils.cpp#L605-L616 - From the code snippets, if the output net is invalid, the flow will be skipped for this output net

Function set_wire_lut_output() is called twice in the code

• https://github.com/lnis-uofu/OpenFPGA/blob/master/openfpga/src/utils/physical_pb_utils.cpp#L640 - which there is pre-checking the to make sure output net is not invalid before it is called - https://github.com/lnis-uofu/OpenFPGA/blob/master/openfpga/src/utils/physical_pb_utils.cpp#L605-L607
• https://github.com/lnis-uofu/OpenFPGA/blob/master/openfpga/src/utils/physical_pb_utils.cpp#L324. However for this particular call, there is no check of output net validity like other code did.

Currently there is potential that the code setting the output as "wire LUT output" even if the output net is AtomNetId::INVALID (or -1). And when this happen, this will raise assertion in later checking stage - https://github.com/lnis-uofu/OpenFPGA/blob/master/openfpga/src/repack/build_physical_truth_table.cpp#L153

What does this pull request change?

Which part of the code base require a change

• VPR
• Tileable routing architecture generator
• OpenFPGA libraries
• FPGA-Verilog
• FPGA-Bitstream
• FPGA-SDC
• FPGA-SPICE
• Flow scripts
• Architecture library
• Cell library
• Documentation
• Regression tests
• Continous Integration (CI) scripts

Impact of the pull request

• Require a change on Quality of Results (QoR)
• Break back-compatibility. If so, please list who may be influenced.

[chungshien]

@alaindargelas

[chungshien]

@tangxifan, I request you to review since I saw you are the one who write/modify the file most in file history.

[tangxifan]

@chungshien

• I understand that an assertation will be thrown when the is_wired_lut() meets exceptions. It is intended to show a stop sign, warning us that resulting bitstream may be wrong. If we let it go in a quiet way, enormous efforts will be spent on spotting the bug.
• Therefore, my preference is to throw an exception or an error message, stopping the bitgen process.
• If you are facing the error in your application, my suggestion is to dig out the reason behind. It could be a bug in repacker, that we should fix, like Fixed a critical bug on repacker where global nets are ignored even there is only 1 valid routing trace #1688

[alaindargelas]

@tangxifan, as @chungshien-chai pointed out, there are 3 places this function is called, 2 are guarded by the "if condition". @chungshien-chai is simply adding the guarding to the 3rd location in this PR.
The root cause of the assert is a "clock" is used as "data" and we are fixing the root cause of our issue in the eblif netlist.
But, for the sake of consistency, we should either remove the guarding from all 3 locations, potentially having the IE happen earlier in the flow or add the guarding proposed by this PR to the 3rd location.

A more precise error message should also be added when a clock is used as data, as bitstream (routing) is illegal.

[tangxifan]

@alaindargelas Thank you for the details. Now I understand the issue you are facing.

• There should be no restrictions that clock is used to drive any LUT or inputs of programmable blocks. If this is the limitation you are seeing, we should fix it in this PR.
• I am o.k. to remove the guarding experimental in this PR and see what testcase you may break. Each of them are adding for a real testcase.
• A new testcase is required where the clock is used as the data input. Just like what I did to test reset signal: https://github.com/lnis-uofu/OpenFPGA/blob/master/openfpga_flow/benchmarks/micro_benchmark/rst_on_lut/rst_on_lut.v

[alaindargelas]

@tangxifan when the architecture file contains no clock to data or data to clock path, then clock used at data should be illegal. When using ideal clock network with no routing elements in the clock that is the case.
There should be a way to check this in the case the arch file separates the clock and data planes

[tangxifan]

@tangxifan when the architecture file contains no clock to data or data to clock path, then clock used at data should be illegal. When using ideal clock network with no routing elements in the clock that is the case. There should be a way to check this in the case the arch file separates the clock and data planes

@alaindargelas This makes sens to me. Sanity checks should be applied before repack. Using existing data structure, I believe it is not difficult.