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.

Sign up for GitHub

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

Jump to bottom

Unconnected CARRY4s #53

Closed
mbpeterson70 opened this issue Mar 18, 2021 · 9 comments
Closed

Unconnected CARRY4s #53

mbpeterson70 opened this issue Mar 18, 2021 · 9 comments
Assignees
@acomodi @mbpeterson70

Comments

@mbpeterson70
Copy link

I've noticed that Fasm2bels does not seem to be connecting carry chains correctly. As an example, I have a design where the CARRY4 A's carry-in pin (CI) should be driven by the MSB of CARRY4 B's CO wire.

CARRY4 A:

  (* KEEP, DONT_TOUCH, BEL = "CARRY4" *)
  CARRY4 #(
  ) CLBLL_L_X2Y110_SLICE_X1Y110_CARRY4 (
.CI(CLBLL_L_X2Y109_SLICE_X1Y109_COUT),
.CO({CLBLL_L_X2Y110_SLICE_X1Y110_D_CY, CLBLL_L_X2Y110_SLICE_X1Y110_C_CY, CLBLL_L_X2Y110_SLICE_X1Y110_B_CY, CLBLL_L_X2Y110_SLICE_X1Y110_A_CY}),
.CYINIT(1'b0),
...

CARRY4B:

  (* KEEP, DONT_TOUCH, BEL = "CARRY4" *)
  CARRY4 #(
  ) CLBLL_L_X2Y109_SLICE_X1Y109_CARRY4 (
.CI(1'b0),
.CO({CLBLL_L_X2Y109_SLICE_X1Y109_D_CY, CLBLL_L_X2Y109_SLICE_X1Y109_C_CY, CLBLL_L_X2Y109_SLICE_X1Y109_B_CY, CLBLL_L_X2Y109_SLICE_X1Y109_A_CY}),
.CYINIT(1'b0),
...

However, CLBLL_L_X2Y109_SLICE_X1Y109_COUT (which is connected to CARRY4 A's CI pin) is not driven by anything, so the CARRY4s remain unconnected. I've been able to provide a simple fix to my files by adding in an assign statement that would look something like this: assign CLBLL_L_X2Y109_SLICE_X1Y109_COUT = CLBLL_L_X2Y109_SLICE_X1Y109_D_CY;

This is an easy fix, but it would be nice to contribute a fix to the code, however, I am not very familiar with the Fasm2bels code. Could someone point me to where I might start looking in the code to provide a fix? Also, does it seem better to have fasm2bels just insert an assign statement like I did or to just remove the "*_COUT" middle man entirely and drive CI of CARRY4 A with the original *_D_CY output of CARRY4 B?
@litghost
Copy link
Contributor

Also, does it seem better to have fasm2bels just insert an assign statement like I did or to just remove the "*_COUT" middle man entirely and drive CI of CARRY4 A with the original *_D_CY output of CARRY4 B?

So this is what the logic in fasm2bels should be doing

@litghost
Copy link
Contributor

@mbpeterson70 Please attach a replicating test case (e.g. a bitstream or FASM file).

@litghost litghost assigned mbpeterson70 and unassigned litghost Mar 18, 2021
@mbpeterson70
Copy link
Author

mbpeterson70 commented Mar 18, 2021 •
edited
Loading

Here is a FASM file as well as what fasm2bels outputs. In cpu8080_f2b.v, an example of two problem CARRY4s can be found on lines 19210-19219 (upper CARRY4 block) and 19065-19074 (lower CARRY4 block).
cpu8080_fasm2bels_files.zip

@mbpeterson70
Copy link
Author

@litghost do you have any advice about where I should start looking in the code to provide a fix?

@jgoeders
Copy link
Contributor

jgoeders commented Apr 2, 2021

Here's a very simple design with the same issue. It's just a single 16-bit adder, implemented as 4x CARRY4:
add16.zip

@e7p
Copy link

e7p commented Apr 5, 2021

Also, does it seem better to have fasm2bels just insert an assign statement like I did or to just remove the "*_COUT" middle man entirely and drive CI of CARRY4 A with the original *_D_CY output of CARRY4 B?

So this is what the logic in fasm2bels should be doing

Commit 514cc91 seems to cause the issue, however I don't really have a clue how to set the input of the next CARRY4 element to D_CY instead of COUT to fix this in an appropriate way.

@mithro
Copy link
Contributor

mithro commented Apr 6, 2021

@acomodi - Can you provide @e7p with some assistance here?

@acomodi
Copy link
Contributor

acomodi commented Apr 7, 2021

@e7p So, I believe that, even though 514cc91 solved an issue for which the CY internal source was not added in case the last bit of the carry chain was used (bug discovered here), that commit does indeed introduce an issue for which COUT will be left unconnected in case.

Looking at the current code though, the COUT source should be added here: https://github.com/SymbiFlow/symbiflow-xc-fasm2bels/blob/99ae85f78cc89d3161d6ab58d64b0337c11e2b77/fasm2bels/models/clb_models.py#L1778-L1780.

Might be worth investigating what is happening there, and if the COUT source is correctly added to the site

@e7p
Copy link

e7p commented Apr 7, 2021

Actually, the problem is, that the COUT source is not correctly added, as there is already one source existing (D_CY) connected to the exact same signal. So effectively maybe it could also make sense to simply connect the two like stated by @mbpeterson70 in the first comment:
assign CLBLL_L_X2Y109_SLICE_X1Y109_COUT = CLBLL_L_X2Y109_SLICE_X1Y109_D_CY;

@the-centry the-centry mentioned this issue Apr 25, 2021
Closed
@jaromharris jaromharris mentioned this issue Jun 17, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@acomodi acomodi

@mbpeterson70 mbpeterson70

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@mithro @litghost @e7p @jgoeders @acomodi @jaromharris @mbpeterson70