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

Clash generates invalid verilog names #2845

Open
christiaanb opened this issue Nov 18, 2024 · 0 comments · May be fixed by #2846
Open

Clash generates invalid verilog names #2845

christiaanb opened this issue Nov 18, 2024 · 0 comments · May be fixed by #2846

Comments

@christiaanb
Copy link
Member

christiaanb commented Nov 18, 2024

Tracking the invalid name generation in a separate issue

I think I have two more reproducers that lead to heaps of duplication, with a clock primitive being involved. I'm using GHC 9.8.2. They generate funky names as well: names like Duplicate.topEntity_cntr as well as \Duplicate.topEntity_cntr. The latter might technically be valid Verilog, but it seems Vivado isn't too happy about it anyway.

The first variant is without NOINLINE cntr:

module Duplicate where

import Clash.Explicit.Prelude
import Clash.Explicit.Testbench

topEntity ::
  Signal System (Unsigned 8)
topEntity = cntr + x
 where
  cntr = register clk noReset enableGen 0 0
  x = register clk noReset enableGen 100 0
  done = (== 100) <$> cntr
  clk = tbClockGen $ not <$> done

This generates the following Verilog:

Duplicate.hs Verilog
/* AUTOMATICALLY GENERATED VERILOG-2001 SOURCE CODE.
** GENERATED BY CLASH 1.9.0. DO NOT MODIFY.
*/
`default_nettype none
`timescale 100fs/100fs
module topEntity
    ( // No inputs

      // Outputs
      output wire [7:0] result
    );
  reg [7:0] c$app_arg = 8'd100;
  // Duplicate.hs:10:3-6
  reg [7:0] \Duplicate.topEntity_cntr  = 8'd0;
  // Duplicate.hs:13:3-5
  wire  \Duplicate.topEntity_clk ;
  // Duplicate.hs:10:3-6
  reg [7:0] Duplicate.topEntity_cntr = 8'd0;
  // Duplicate.hs:13:3-5
  wire  Duplicate.topEntity_clk;
  wire  c$rst;
  wire  c$rst_0;
  wire  c$rst_1;

  assign c$rst = (1'b0);

  // register begin
  always @(posedge Duplicate.topEntity_clk or  posedge  c$rst) begin : DuplicatetopEntity_cntr_register
    if ( c$rst) begin
      Duplicate.topEntity_cntr <= 8'd0;
    end else begin
      Duplicate.topEntity_cntr <= 8'd0;
    end
  end
  // register end

  // tbClockGen begin
  // pragma translate_off
  reg  clk;
  // 1 = 0.1ps
  localparam half_period = (100000 / 2);
  always begin
    // Delay of 1 mitigates race conditions (https://github.com/steveicarus/iverilog/issues/160)
    #1 clk =  0 ;
    #1000000 forever begin

      if (~ (~ (Duplicate.topEntity_cntr == 8'd100))) begin
        `ifdef VERILATOR
          $c("std::exit(0);");
        `endif
        $finish(0);
      end

      clk = ~ clk;
      #half_period;
      clk = ~ clk;
      #half_period;
    end
  end

  assign Duplicate.topEntity_clk = clk;
  // pragma translate_on
  // tbClockGen end

  assign c$rst_0 = (1'b0);

  // register begin
  always @(posedge Duplicate.topEntity_clk or  posedge  c$rst_0) begin : c$app_arg_register
    if ( c$rst_0) begin
      c$app_arg <= 8'd100;
    end else begin
      c$app_arg <= 8'd0;
    end
  end
  // register end

  assign c$rst_1 = (1'b0);

  // register begin
  always @(posedge \Duplicate.topEntity_clk  or  posedge  c$rst_1) begin : DuplicatetopEntity_cntr_register_0
    if ( c$rst_1) begin
      \Duplicate.topEntity_cntr  <= 8'd0;
    end else begin
      \Duplicate.topEntity_cntr  <= 8'd0;
    end
  end
  // register end

  // tbClockGen begin
  // pragma translate_off
  reg  clk_0;
  // 1 = 0.1ps
  localparam half_period_0 = (100000 / 2);
  always begin
    // Delay of 1 mitigates race conditions (https://github.com/steveicarus/iverilog/issues/160)
    #1 clk_0 =  0 ;
    #1000000 forever begin

      if (~ (~ (\Duplicate.topEntity_cntr  == 8'd100))) begin
        `ifdef VERILATOR
          $c("std::exit(0);");
        `endif
        $finish(0);
      end

      clk_0 = ~ clk_0;
      #half_period_0;
      clk_0 = ~ clk_0;
      #half_period_0;
    end
  end

  assign \Duplicate.topEntity_clk  = clk_0;
  // pragma translate_on
  // tbClockGen end

  assign result = \Duplicate.topEntity_cntr  + c$app_arg;


endmodule

Al least the following things are duplicated:

  reg [7:0] \Duplicate.topEntity_cntr  = 8'd0;
  wire  \Duplicate.topEntity_clk ;
  reg [7:0] Duplicate.topEntity_cntr = 8'd0;
  wire  Duplicate.topEntity_clk;

  assign Duplicate.topEntity_clk = clk;

  assign \Duplicate.topEntity_clk  = clk_0;

along with 2 instead of 1 clock primitive, and 2 register blocks for cntr instead of 1.

[edit]
I noticed I could get rid of the second clock and still have the duplication. The one with the second clock might still be interesting because of other weirdness, such as the done signal in HDL being not <$> done in Haskell?

  // tbClockGen begin
  [...]
      if (~ Duplicate.topEntity_done) begin
        `ifdef VERILATOR
          $c("std::exit(0);");
        `endif
        $finish(0);
      end

  assign Duplicate.topEntity_done = ~ (Duplicate.topEntity_cntr == 8'd100);
This is output generated by the following code:
module Duplicate where

import Clash.Explicit.Prelude
import Clash.Explicit.Testbench

topEntity ::
  Signal System (Unsigned 8)
topEntity = cntr + posD
 where
  cntr = register clk1 noReset enableGen 0 0
  posD = register clk2 noReset enableGen 0 0
  done = (== 100) <$> cntr
  (clk1, clk2) = biTbClockGen $ not <$> done
[/edit]

Originally posted by @DigitalBrains1 in #2570 (comment)

christiaanb added a commit that referenced this issue Nov 18, 2024
The global vars are usually backed by a clock generator that
are not work-free.

In addition, when these global vars are recursively defined,
they can mess up the post-normalization flattening stage which
then violates certain invariants of the netlist generation stage.
This then causes the netlist generation stage to generate bad
Verilog names.

Fixes #2845
@christiaanb christiaanb linked a pull request Nov 18, 2024 that will close this issue
2 tasks
christiaanb added a commit that referenced this issue Nov 18, 2024
The global vars are usually backed by a clock generator that
are not work-free.

In addition, when these global vars are recursively defined,
they can mess up the post-normalization flattening stage which
then violates certain invariants of the netlist generation stage.
This then causes the netlist generation stage to generate bad
Verilog names.

Fixes #2845
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 a pull request may close this issue.

1 participant