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

Sync upstream #74

Closed
wants to merge 1,457 commits into from
Closed

Sync upstream #74

wants to merge 1,457 commits into from

Conversation

acomodi
Copy link

@acomodi acomodi commented Apr 17, 2020

There was an error in the previous upstream merge, as the master was not completely up to date with upstream.

What is new is the following:

  • .lut and .box from techlib/xilinx has been removed: all the abc related files are now autogenerated. I have added the delay information followin the same pattern as the normal CARRY4 (with the necessary difference) to resemble the old .box definition.

  • in the synthesis script of xilinx there needs to be the -specify option when reading the cells_sim.v file. This adds the delay information to the auto-generated .lut and .box files.

I am running now tests from symbiflow arch-defs without the -nowidelut option that I needed to add, to see if that is solved now.

ABC now works fine also with the mfs step, which was removed in the previous upstream sync.

whitequark and others added 30 commits April 3, 2020 05:51
"techmap -map +/cmp2lcu.v" for decomposing arithmetic compares to $lcu
Co-Authored-By: whitequark <whitequark@whitequark.org>
Co-Authored-By: David Shah <dave@ds0.me>
@litghost
Copy link

litghost commented Apr 23, 2020

By removing the ABC map of those SRL, the all_xc7 passes.
Now there is an issue with the dram_test_32x1d_vivado_diff_fasm.

This should be due to the following fix that happened upstream on the 32x1d :

diff --git a/techlibs/xilinx/abc9_map.v b/techlibs/xilinx/abc9_map.v
index cff57fda..476eb96d 100644
--- a/techlibs/xilinx/abc9_map.v
+++ b/techlibs/xilinx/abc9_map.v
@@ -399,7 +399,7 @@ module RAM32X1D (
     .DPRA0(DPRA0), .DPRA1(DPRA1), .DPRA2(DPRA2), .DPRA3(DPRA3), .DPRA4(DPRA4)
   );
   $__ABC9_RAM6 spo (.A($SPO), .S({1'b1, A4, A3, A2, A1, A0}), .Y(SPO));
+  $__ABC9_RAM6 dpo (.A($DPO), .S({1'b1, DPRA4, DPRA3, DPRA2, DPRA1, DPRA0}), .Y(DPO));
-  $__ABC9_RAM6 dpo (.A($DPO), .S({1'b1, A4, A3, A2, A1, A0}), .Y(DPO));
 endmodule

It seems that the CX, D6 and C6 pins are not constrained and is the one causing problems. IMO this should be solved on the arch-defs side.

Need to investigate this.

This bug is almost certainly an error in the BLIF, not in VPR. Whether the error is in upstream Yosys or the symbiflow-arch-defs techmap is unclear.

If I had to guess what the issue is, I believe the issue at hand is related to illegal LUT rotations w.r.t. :

> $__ABC9_RAM6 spo (.A($SPO), .S({1'b1, A4, A3, A2, A1, A0}), .Y(SPO));
> $__ABC9_RAM6 dpo (.A($DPO), .S({1'b1, DPRA4, DPRA3, DPRA2, DPRA1, DPRA0}), .Y(DPO));

I believe the correct ABC9 representation is a ABC9_RAM5, not ABC_RAM6. This is because the high address line must but connected to the high LUT pin for the LUT6 mux to stay high. I can provide diagrams explaining more, but I believe the solution here is to change ABC9_RAM6 -> ABC9_RAM5, and drop the 1'b1 from the address set. If for whatever reason a ABC9_RAM6 is required here, then a directive to abc9 is required to indicate that the high address line is not equivalent to the other 5 address lines.

@acomodi
Copy link
Author

acomodi commented Apr 23, 2020

@litghost Thanks for the insight. Turns out that Yosys does not support ABC9_RAM5, by adding the support for it the test passes correctly. I'll try to get this upstream. For now I will integrate the change in this PR in yet another wip branch

litghost and others added 17 commits April 23, 2020 18:04
Signed-off-by: Keith Rothman <537074+litghost@users.noreply.github.com>
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
Signed-off-by: Keith Rothman <537074+litghost@users.noreply.github.com>
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
Signed-off-by: Keith Rothman <537074+litghost@users.noreply.github.com>
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
Signed-off-by: Keith Rothman <537074+litghost@users.noreply.github.com>
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
…ble_ramb18e_w2', 'wip/fix-ram32x1d-abc' and 'wip/remove-srl-abc9-map' into sync_upstream
This is an Octopus Merge commit of the following branches:

wip/carry4-cout
wip/disable-primitives
wip/disable_ramb18e_w2
wip/fix-ram32x1d-abc
wip/remove-srl-abc9-map

Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
@litghost
Copy link

> module SRLC16E (
>   output Q, Q15,
>   (* techmap_autopurge *) input A0, A1, A2, A3, CE, CLK, D
> );
>   parameter [15:0] INIT = 16'h0000;
>   parameter [0:0] IS_CLK_INVERTED = 1'b0;
>   wire $Q;
>   SRLC16E #(
>     .INIT(INIT), .IS_CLK_INVERTED(IS_CLK_INVERTED)
>   ) _TECHMAP_REPLACE_ (
>     .Q($Q), .Q(Q15),
>     .A0(A0), .A1(A1), .A2(A2), .A3(A3), .CE(CE), .CLK(CLK), .D(D)
>   );
>   $__ABC9_RAM6 q (.A($Q), .S({1'b1, A3, A2, A1, A0, 1'b1}), .Y(Q));
> endmodule

The SRL errors are likely exact same failure. ABC is likely rotating the high and low pins, which again are illegal rotations.

@acomodi
Copy link
Author

acomodi commented Apr 23, 2020

@litghost Yep, trying to apply the same fix there. But I think this should apply to SRL16 only?

@litghost
Copy link

@litghost Yep, trying to apply the same fix there. But I think this should apply to SRL16 only?

I believe there is a similar issue with the SRL32. Basically anytime you see a 1'b1 or 1'b0 in a ABC_RAM6 that is likely a bug.

@acomodi
Copy link
Author

acomodi commented Apr 23, 2020

Unfortunately the same fix for the RAM32X1D using RAM5 does not work (both on SRL16 and SRL32), the testbench still fails. I have currently produced a new master+wip that does not have the SRLC[16|32]E abc9 map and the RAM32X1D fix. Running xc7_diff_fasm now.

I am trying to make a super simple version of the testbench to prove the problem and open an issue upstream.

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

Successfully merging this pull request may close these issues.