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

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

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

chungshien
Copy link
Collaborator

@chungshien chungshien commented Jun 19, 2024

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:

Function is_wire_lut_output() is called twice in the code

Function set_wire_lut_output() is called twice in the code

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 chungshien changed the title Openfpga wire lut output Make sure net is valid before setting it as wire LUT output Jun 19, 2024
@chungshien
Copy link
Collaborator Author

@alaindargelas

@chungshien
Copy link
Collaborator Author

chungshien commented Jun 19, 2024

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

@tangxifan
Copy link
Collaborator

@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
Copy link

@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
Copy link
Collaborator

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

@alaindargelas
Copy link

@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
Copy link
Collaborator

@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.

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

Successfully merging this pull request may close these issues.

4 participants