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

Simple circuit composition leads to logic loops #72

Closed
rowanG077 opened this issue Mar 29, 2024 · 3 comments
Closed

Simple circuit composition leads to logic loops #72

rowanG077 opened this issue Mar 29, 2024 · 3 comments

Comments

@rowanG077
Copy link
Member

rowanG077 commented Mar 29, 2024

In the clash ethernet project we ran into issues with logic loops during synthesis which were not caught in the test suite. After some minimization we can get even simple compositions to warn about a logic loop. I'm not confident they are not false positives but on the other hand I have not yet seen false positive logic loops in yosys.

topEntity
  :: Clock System
  -> Reset System
  -> (Signal System (Df.Data Bit), Signal System Ack)
  -> (Signal System Ack, Signal System (Df.Data Bit))
topEntity clk rst = toSignals $ exposeClockResetEnable (Df.fifo d1 |> Df.registerFwd) clk rst enableGen

Which yosys reports as a logic loop:

2.11. Executing CHECK pass (checking for obvious problems).
Checking module topEntity...
Warning: found logic loop in module topEntity:
    cell $ternary$verilog/Clash.Lattice.ECP5.Colorlight.TopEntity.topEntity/topEntity.v:112$236 ($mux)
    cell $ternary$verilog/Clash.Lattice.ECP5.Colorlight.TopEntity.topEntity/topEntity.v:114$238 ($mux)
    cell $ternary$verilog/Clash.Lattice.ECP5.Colorlight.TopEntity.topEntity/topEntity.v:77$228 ($mux)
    cell $ternary$verilog/Clash.Lattice.ECP5.Colorlight.TopEntity.topEntity/topEntity.v:99$232 ($mux)
    wire \c$case_alt [2]
    wire \c$ds1_case_alt [0]
    wire \c$ds8_case_alt
    wire \ds8
Warning: found logic loop in module topEntity:
    cell $ternary$verilog/Clash.Lattice.ECP5.Colorlight.TopEntity.topEntity/topEntity.v:112$236 ($mux)
    cell $ternary$verilog/Clash.Lattice.ECP5.Colorlight.TopEntity.topEntity/topEntity.v:114$238 ($mux)
    cell $ternary$verilog/Clash.Lattice.ECP5.Colorlight.TopEntity.topEntity/topEntity.v:77$228 ($mux)
    cell $ternary$verilog/Clash.Lattice.ECP5.Colorlight.TopEntity.topEntity/topEntity.v:99$232 ($mux)
    wire \c$case_alt [2]
    wire \c$ds1_case_alt [1]
    wire \c$ds8_case_alt
    wire \ds8
Found and reported 2 problems.
@christiaanb
Copy link
Member

I'm not seeing it in the code nor in the RTL viewer of quartus. Could you post an SVG output of yosys?

@rowanG077
Copy link
Member Author

rowanG077 commented Apr 17, 2024

This is the verilog output we get:

/* AUTOMATICALLY GENERATED VERILOG-2001 SOURCE CODE.
** GENERATED BY CLASH 1.6.4. DO NOT MODIFY.
*/
`timescale 100fs/100fs
module topEntity
    ( // Inputs
      input  clk // clock
    , input  rst // reset
    , input [2:0] in

      // Outputs
    , output wire [2:0] out
    );
  reg [1:0] c$ds8_app_arg = {1'b0,1'bx};
  wire  bwd;
  wire [2:0] c$case_alt;
  wire [2:0] result;
  wire [1:0] s2rAc;
  wire [1:0] c$case_alt_0;
  wire [1:0] c$app_arg;
  wire  c$app_arg_0;
  wire [4:0] c$case_alt_1;
  wire [1:0] iDat;
  wire [2:0] result_0;
  wire [5:0] c$ds1_case_alt;
  wire [1:0] c$ds1_app_arg;
  wire  brRead1;
  wire  c$ds1_app_arg_0;
  wire [1:0] c$ds1_app_arg_1;
  wire [0:0] ds8;
  wire [0:0] c$ds8_case_alt;
  wire  c$ds8_case_scrut;
  wire [0:0] amtLeft2;
  wire [1:0] ds7;
  wire [1:0] c$ds7_case_alt;
  wire  push;
  wire [0:0] amtLeft1;
  wire [0:0] ds6;
  wire [1:0] maybePush;
  wire [1:0] c$maybePush_case_alt;
  wire  a1;
  wire  inpB;
  wire [1:0] c$ds_app_arg;
  wire  result_1;
  wire  wrdata;
  reg [1:0] ds = {1'b0,   {1 {1'bx}}};
  reg  result_2;
  wire  c$app_arg_1;
  wire  a1_0;
  wire  c$app_arg_3;
  wire [0:0] amtLeft0;
  reg [0:0] ds_0 = 1'd1;
  wire [1:0] s2rAc_0;
  wire  r2sAc;
  wire [1:0] c$ds_app_arg_selection_2;
  wire [1:0] c$app_arg_selection_4;
  wire [1:0] c$app_arg_selection_12;

  // register begin
  always @(posedge clk or  posedge  rst) begin : c$ds8_app_arg_register
    if ( rst) begin
      c$ds8_app_arg <= {1'b0,1'bx};
    end else begin
      c$ds8_app_arg <= c$case_alt_1[4:3];
    end
  end
  // register end

  assign bwd = c$case_alt_1[2:2];

  assign c$case_alt = (rst) ? {1'b0,
                               {1'b0,1'bx}} : {bwd,   s2rAc};

  assign result = {c$case_alt[2:2],
                   c$case_alt_1[1:0]};

  assign s2rAc = result_0[1:0];

  assign c$case_alt_0 = r2sAc ? iDat : c$ds8_app_arg;

  assign c$app_arg = c$ds8_app_arg[1:1] ? c$case_alt_0 : iDat;

  assign c$app_arg_0 = c$ds8_app_arg[1:1] ? r2sAc : 1'b1;

  assign c$case_alt_1 = {c$app_arg,
                         {c$app_arg_0,   c$ds8_app_arg}};

  assign iDat = c$case_alt[1:0];

  assign result_0 = {c$ds1_case_alt[2:2],
                     c$ds1_case_alt[1:0]};

  assign c$ds1_case_alt = (rst) ? {1'd1,
                                   {{1'b0,1'bx},   1'b0,   {1'b0,1'bx}}} : {ds8,
                                                                            {c$ds1_app_arg_1,   c$ds1_app_arg_0,
                                                                             c$ds1_app_arg}};

  assign c$ds1_app_arg = c$ds8_case_scrut ? {1'b1,brRead1} : {1'b0,1'bx};

  assign brRead1 = ds7[1:1];

  assign c$ds1_app_arg_0 = maybePush[1:1] ? 1'b1 : 1'b0;

  assign c$ds1_app_arg_1 = maybePush[1:1] ? {1'b1,push} : {1'b0,1'bx};

  assign ds8 = c$ds8_case_scrut ? c$ds8_case_alt : amtLeft1;

  assign c$ds8_case_alt = inpB ? (amtLeft1 + 1'd1) : amtLeft1;

  assign c$ds8_case_scrut = amtLeft2 < 1'd1;

  assign amtLeft2 = ds7[0:0];

  assign ds7 = maybePush[1:1] ? c$ds7_case_alt : {result_1,
                                                  amtLeft0};

  assign c$ds7_case_alt = (amtLeft0 == 1'd1) ? {push,
                                                amtLeft1} : {result_1,   amtLeft0};

  assign push = maybePush[0:0];

  assign amtLeft1 = ds6;

  assign ds6 = maybePush[1:1] ? (amtLeft0 - 1'd1) : amtLeft0;

  assign maybePush = (amtLeft0 > 1'd0) ? c$maybePush_case_alt : {1'b0,1'bx};

  assign c$maybePush_case_alt = s2rAc_0[1:1] ? {1'b1,a1} : {1'b0,1'bx};

  assign a1 = s2rAc_0[0:0];

  assign inpB = result[2:2];

  assign c$ds_app_arg_selection_2 = c$ds1_case_alt[4:3];

  assign c$ds_app_arg = c$ds_app_arg_selection_2[1:1] ? {1'b1,
                                                         wrdata} : {1'b0,   {1 {1'bx}}};

  assign result_1 = ds[1:1] ? ds[0:0] : result_2;

  assign wrdata = c$ds1_case_alt[3:3];

  // register begin
  always @(posedge clk or  posedge  rst) begin : ds_register
    if ( rst) begin
      ds <= {1'b0,   {1 {1'bx}}};
    end else begin
      ds <= c$ds_app_arg;
    end
  end
  // register end

  // blockRamU begin
  reg  result_2_RAM [0:1-1];


  always @(posedge clk) begin : result_2_blockRam
    if (c$app_arg_3) begin
      result_2_RAM[(64'sd0)] <= c$app_arg_1;
    end
    result_2 <= result_2_RAM[(64'sd0)];
  end
  // blockRamU end

  assign c$app_arg_selection_4 = c$ds1_case_alt[4:3];

  assign c$app_arg_1 = c$app_arg_selection_4[1:1] ? a1_0 : ({1 {1'bx}});

  assign a1_0 = c$ds1_case_alt[3:3];

  assign c$app_arg_selection_12 = c$ds1_case_alt[4:3];

  assign c$app_arg_3 = c$app_arg_selection_12[1:1] ? 1'b1 : 1'b0;

  assign amtLeft0 = ds_0;

  // register begin
  always @(posedge clk or  posedge  rst) begin : ds_0_register
    if ( rst) begin
      ds_0 <= 1'd1;
    end else begin
      ds_0 <= c$ds1_case_alt[5:5];
    end
  end
  // register end

  assign out = {result_0[2:2],   result[1:0]};

  assign s2rAc_0 = in[2:1];

  assign r2sAc = in[0:0];


endmodule

These are the commands yosys runs before reporting the logic loop:

read_verilog <put TopEntity filepath here>
proc
flatten
tribuf -logic
deminout
opt_expr
opt_clean
check

Below is the svg (well PNG but whatever) out from yosys after the check

yosys

And here is the same image with the logic loop marked in red

yosys2

You can follow the graph directly in the verilog. Technically it is a logic loop if you view muxes as black boxes but I don't think it should be reported be since: ds8 -> Becomes Bit 5 of c$ds1_case_alt through a mux -> Bit 5 is no longer used in the path. Essentially it is the same as this:

module topEntity
    ( input in

      // Outputs
    , output out
    );

  wire [1:0] foo;
  wire bar;

 assign foo = in ? 2'b00 : {bar, 1'b1};
 assign bar = foo[0];

 assign out = bar;

endmodule

@rowanG077
Copy link
Member Author

This was very recently fixed in yosys in YosysHQ/yosys#4184

It must not have been bug in yosys for long since I would expect I would have hit it much more often using clash.

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

No branches or pull requests

2 participants