search

SymbiFlow / yosys

SymbiFlow WIP changes for Yosys Open SYnthesis Suite
http://www.clifford.at/yosys/
ISC License
37 stars 9 forks source link

Sync upstream #74

Closed acomodi closed 3 years ago

acomodi commented 4 years ago

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:

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.

acomodi commented 4 years ago

@litghost Just checked the build. VPR fails as the blif contains a $_TBUF_ primitive which is unsupported

litghost commented 4 years ago

@litghost Just checked the build. VPR fails as the blif contains a $_TBUF_ primitive which is unsupported

That indicates that is an iopad that didn't get mapped on a primative.

litghost commented 4 years ago

Was the failure on a ROI or ROI less design? If it was a ROI based design, ROI based designs cannot support tristate inout's, which would be the problem.

acomodi commented 4 years ago

I need to check that in more detail to understand if there was a techmap failure, but it was a non-ROI design. Baselitex to be precise, and the signal was an inout

acomodi commented 4 years ago

By analyizing the error I found out that the eth_mdio inout toplevel pad did not get correctly translated into an IOBUF by yosys.

All the other inouts do have explicit IOBUF instances in the baselitex design, therefore they do not incur in the wrong translation.

mithro commented 4 years ago

@acomodi IIRC there was a change in upstream liteeth which fixed the eth_mdio behaviour you are seeing above. Maybe @enjoy-digital knows?

acomodi commented 4 years ago

Update:

There was a confiuration error in arch-defs when I ran the last tests, and yosys got back to the conda version, so the TBUF error was due to the non-default iopad map.

Two additional patches needed to be made:

litghost commented 4 years ago
  • disable mfs pass in ABC (the error showed up once again): disabled in c62cce8

What is the implication of disabling mfs? Can you replicate the mfs error and create an issue in YosysHQ?

litghost commented 4 years ago
  • extend the CARRY_COUT in the LCU definition as well in arith_map.v: unsure whether this is needed, but there was a change in the arith_map.v upstream that causes MUXCY to be inferred. This patch seems to solve the issue: 5959da5

This is fine.

acomodi commented 4 years ago

What is the implication of disabling mfs? Can you replicate the mfs error and create an issue in YosysHQ?

Forgot to mention there is already an issue about this opened a few days ago, so it is a known issue: https://github.com/YosysHQ/yosys/issues/1962

litghost commented 4 years ago

Great, so then what is the current status of this PR?

acomodi commented 4 years ago

I still need to find a workaround for https://github.com/YosysHQ/yosys/issues/1970, otherwise some tests in arch-defs that explicitly instantiate LUTs may fail.

acomodi commented 4 years ago

https://github.com/YosysHQ/yosys/issues/1970 Got solved and the fix merged on upstream master, we need to update the SymbiFlow/yosys master branch (I do not have access to merge do this though).

With that fix, I think we can update master+wip.

litghost commented 4 years ago

I do not have access to merge do this though

Permissions should be fixed!

acomodi commented 4 years ago

I have found what causes issues with the srl16_chain testbench. In the past months, SRLC16E and SRLC32E support was added, with also a dedicated abc techmap:

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

module SRLC32E (
  output Q,
  output Q31,
  (* techmap_autopurge *) input [4:0] A,
  (* techmap_autopurge *) input CE, CLK, D
);
  parameter [31:0] INIT = 32'h00000000;
  parameter [0:0] IS_CLK_INVERTED = 1'b0;
  wire $Q;
  SRLC32E #(
    .INIT(INIT), .IS_CLK_INVERTED(IS_CLK_INVERTED)
  ) _TECHMAP_REPLACE_ (
    .Q($Q), .Q31(Q31),
    .A(A), .CE(CE), .CLK(CLK), .D(D)
  );
  $__ABC9_RAM6 q (.A($Q), .S({1'b1, A}), .Y(Q));
endmodule

These two techmap replace modules seem to cause issues with the testbenches. By removing them, testbench correctly passes.

litghost commented 4 years ago

I have found what causes issues with the srl16_chain testbench. In the past months, SRLC16E and SRLC32E support was added, with also a dedicated abc techmap:

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

module SRLC32E (
  output Q,
  output Q31,
  (* techmap_autopurge *) input [4:0] A,
  (* techmap_autopurge *) input CE, CLK, D
);
  parameter [31:0] INIT = 32'h00000000;
  parameter [0:0] IS_CLK_INVERTED = 1'b0;
  wire $Q;
  SRLC32E #(
    .INIT(INIT), .IS_CLK_INVERTED(IS_CLK_INVERTED)
  ) _TECHMAP_REPLACE_ (
    .Q($Q), .Q31(Q31),
    .A(A), .CE(CE), .CLK(CLK), .D(D)
  );
  $__ABC9_RAM6 q (.A($Q), .S({1'b1, A}), .Y(Q));
endmodule

These two techmap replace modules seem to cause issues with the testbenches. By removing them, testbench correctly passes.

Is that in cells_map or cells_sim? Anyways, with this, can you generate a reduced test case?

acomodi commented 4 years ago

@litghost This is in abc9_map.v and I am still trying to get a reduced test case, most probably the issues are only related to the SRLC[16|32]E

acomodi commented 4 years ago

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

fasm2bels fails to produce top_bit.v with the following error:

    allow_orphan_sinks=allow_orphan_sinks
  File "/data/tmp/symbiflow-arch-defs/xc/xc7/fasm2bels/make_routes.py", line 453, in expand_sink
    allow_orphan_sinks=allow_orphan_sinks
  File "/data/tmp/symbiflow-arch-defs/xc/xc7/fasm2bels/make_routes.py", line 453, in expand_sink
    allow_orphan_sinks=allow_orphan_sinks
  [Previous line repeated 5 more times]
  File "/data/tmp/symbiflow-arch-defs/xc/xc7/fasm2bels/make_routes.py", line 619, in expand_sink
    assert False, (sink_node_pkey, tile_name, wire_name, sink_wire_pkey)
AssertionError: (248613, 'INT_L_X0Y113', 'SW6END0', 2376540)

If orphan sinks are allowed, than the fasm diff target fails with this diffrent bits set:

--- /data/tmp/symbiflow-arch-defs/build/xc/xc7/tests/dram_test/dram_test_32x1d/artix7-xc7a50t-basys3-roi-virt-xc7a50t-basys3-test/top.bit.fasm      2020-04-23 15:56:16.255560589 +0200
+++ /data/tmp/symbiflow-arch-defs/build/xc/xc7/tests/dram_test/dram_test_32x1d/artix7-xc7a50t-basys3-roi-virt-xc7a50t-basys3-test/design_dram_test_32x1d_vivado.bit.fasm    2020-04-23 15:57:32.365480380 +0200
@@ -2655,7 +2655,7 @@
 INT_L_X10Y119.BYP_ALT0.LOGIC_OUTS_L12
 INT_L_X10Y119.BYP_ALT1.WR1END1
 INT_L_X10Y119.BYP_ALT2.LOGIC_OUTS_L20
-INT_L_X10Y119.BYP_ALT3.SW2END2
+INT_L_X10Y119.BYP_ALT3.WW2END2
 INT_L_X10Y119.BYP_ALT4.BYP_BOUNCE_N3_7
 INT_L_X10Y119.BYP_ALT6.WL1END3
 INT_L_X10Y119.BYP_ALT7.LOGIC_OUTS_L21
@@ -3166,6 +3166,7 @@
 INT_L_X12Y119.WL1BEG1.SW2END2
 INT_L_X12Y119.WL1BEG2.SS2END3
 INT_L_X12Y119.WW2BEG1.LOGIC_OUTS_L13
+INT_L_X12Y119.WW2BEG2.SR1END2
 INT_L_X12Y119.WW2BEG3.LOGIC_OUTS_L15

 INT_L_X12Y120.BYP_ALT0.EL1END0
@@ -3226,6 +3227,7 @@
 INT_L_X12Y120.SL1BEG2.SW2END2
 INT_L_X12Y120.SL1BEG3.SE2END3
 INT_L_X12Y120.SR1BEG1.SL1END0
+INT_L_X12Y120.SR1BEG2.WW4END2
 INT_L_X12Y120.SR1BEG_S0.LOGIC_OUTS_L15
 INT_L_X12Y120.SS2BEG0.EE4END0
 INT_L_X12Y120.SS2BEG1.LOGIC_OUTS_L13
@@ -3237,7 +3239,6 @@
 INT_L_X12Y120.WL1BEG1.LOGIC_OUTS_L20
 INT_L_X12Y120.WL1BEG2.LOGIC_OUTS_L21
 INT_L_X12Y120.WR1BEG2.LOGIC_OUTS_L23
-INT_L_X12Y120.WR1BEG3.WW4END2
 INT_L_X12Y120.WR1BEG_S0.LOGIC_OUTS_L15
 INT_L_X12Y120.WW2BEG1.WW4END2
 INT_L_X12Y120.WW2BEG2.WR1END3

This a dcp view in Vivado: Screenshot from 2020-04-23 16-26-31

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.

litghost commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago 
> 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 commented 4 years ago

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

litghost commented 4 years ago

@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 commented 4 years ago

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.