clash-lang / clash-compiler

Haskell to VHDL/Verilog/SystemVerilog compiler
https://clash-lang.org/
Other
1.4k stars 149 forks source link

Clock primitive is duplicated #2570

Open rowanG077 opened 11 months ago

rowanG077 commented 11 months ago

Tested on current master branch with following GHC versions:

Consider the following TopEntity:

{-# LANGUAGE QuasiQuotes #-}

module TopEntity where

import qualified Clash.Prelude as P
import Clash.Explicit.Prelude
import Clash.Annotations.Primitive
import Data.String.Interpolate (i)

oscg :: Clock System
oscg = clockGen
{-# ANN oscg (InlineYamlPrimitive [Verilog] [i|
  BlackBox:
    name: TopEntity.oscg
    kind: Declaration
    template: |-
      // OSCG begin
      OSCG \#( .DIV(6) ) ~GENSYM[oscillator][0] ( .OSC(~RESULT) );
      // OSCG end
  |]) #-}
{-# NOINLINE oscg #-}

topEntity = s1
  where
    clk = oscg

    f = P.exposeClockResetEnable (P.register False) clk noReset enableGen
    s0 = f $ dflipflop clk (pure True)
    s1 = f s0

And running clash TopEntity.hs --verilog you can see that OSCG primitive is duplicated 3 times:

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

      // Outputs
      output wire  result // reset
    );
  // TopEntity.hs:27:5
  wire  rst;
  reg  result_1 = 1'b0;
  wire  c$app_arg;
  // TopEntity.hs:27:5
  wire  rst_0;
  reg  result_2 = 1'b0;
  wire  c$app_arg_0;
  reg  c$app_arg_1 = ({1 {1'bx}});
  wire  c$app_arg_2;

  assign rst = 1'b0;

  // register begin
  always @(posedge c$app_arg or  posedge  rst) begin : result_1_register
    if ( rst) begin
      result_1 <= 1'b0;
    end else begin
      result_1 <= result_2;
    end
  end
  // register end

  // OSCG begin
  OSCG #( .DIV(6) ) oscillator ( .OSC(c$app_arg) );
  // OSCG end

  assign rst_0 = 1'b0;

  // register begin
  always @(posedge c$app_arg_0 or  posedge  rst_0) begin : result_2_register
    if ( rst_0) begin
      result_2 <= 1'b0;
    end else begin
      result_2 <= c$app_arg_1;
    end
  end
  // register end

  // OSCG begin
  OSCG #( .DIV(6) ) oscillator_0 ( .OSC(c$app_arg_0) );
  // OSCG end

  // delay begin
  always @(posedge c$app_arg_2) begin : c$app_arg_1_delay
    c$app_arg_1 <= 1'b1;
  end
  // delay end

  // OSCG begin
  OSCG #( .DIV(6) ) oscillator_1 ( .OSC(c$app_arg_2) );
  // OSCG end

  assign result = result_1;

endmodule

Per @christiaanb suggestion I added a GHC.Magic.lazy as a view pattern to the Clock argument for:

With no effect.

An interesting observation is that if you rewrite f in the failing code from

f = P.exposeClockResetEnable (P.register False) clk noReset enableGen

to

f = register clk noReset enableGen False

there is no longer any duplication going on.

Removing dflipflop removes one duplication but this is just because there is one less usage of the clock.

christiaanb commented 10 months ago

Does adding a NOINLINE to clk work around the problem?

topEntity = s1
  where
    clk = oscg
    {-# NOINLINE clk #-}

    f = P.exposeClockResetEnable (P.register False) clk noReset enableGen
    s0 = f $ dflipflop clk (pure True)
    s1 = f s0
rowanG077 commented 10 months ago

Well it no longer duplicates the primitive... The primitive is not instantiated at all and clash has made up a clock out of thin air.

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

      // Outputs
      output wire  result
    );
  // TopEntity.hs:28:5
  wire  rst;
  reg  result_2 = 1'b0;
  reg  c$app_arg = ({1 {1'bx}});
  // TopEntity.hs:28:5
  wire  rst_0;
  reg  result_1 = 1'b0;

  assign rst = 1'b0;

  // register begin
  always @(posedge TopEntity.topEntity_clk or  posedge  rst) begin : result_2_register
    if ( rst) begin
      result_2 <= 1'b0;
    end else begin
      result_2 <= c$app_arg;
    end
  end
  // register end

  // delay begin
  always @(posedge TopEntity.topEntity_clk) begin : c$app_arg_delay
    c$app_arg <= 1'b1;
  end
  // delay end

  assign rst_0 = 1'b0;

  // register begin
  always @(posedge TopEntity.topEntity_clk or  posedge  rst_0) begin : result_1_register
    if ( rst_0) begin
      result_1 <= 1'b0;
    end else begin
      result_1 <= result_2;
    end
  end
  // register end

  assign result = result_1;

endmodule
christiaanb commented 10 months ago

Ah, alright...

So a work-around on GHC 9.0.2 that does work is to eta-expand f:

topEntity = s1
  where
    clk = oscg

    f x = P.exposeClockResetEnable (P.register False) clk noReset enableGen x
    s0 = f $ dflipflop clk (pure True)
    s1 = f s0

produces:

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

      // Outputs
      output wire  result
    );
  reg  topEntity_f_result = 1'b0;
  reg  topEntity_f_c$app_arg = 1'b0;
  reg  topEntity_f_c$app_arg_0 = ({1 {1'bx}});
  wire  c$app_arg;
  wire  c$app_arg_0;

  // register begin
  always @(posedge c$app_arg_0 or  posedge  c$app_arg) begin : topEntity_f_result_register
    if ( c$app_arg) begin
      topEntity_f_result <= 1'b0;
    end else begin
      topEntity_f_result <= topEntity_f_c$app_arg;
    end
  end
  // register end

  // register begin
  always @(posedge c$app_arg_0 or  posedge  c$app_arg) begin : topEntity_f_c$app_arg_register
    if ( c$app_arg) begin
      topEntity_f_c$app_arg <= 1'b0;
    end else begin
      topEntity_f_c$app_arg <= topEntity_f_c$app_arg_0;
    end
  end
  // register end

  // delay begin
  always @(posedge c$app_arg_0) begin : topEntity_f_c$app_arg_0_delay
    topEntity_f_c$app_arg_0 <= 1'b1;
  end
  // delay end

  assign c$app_arg = 1'b0;

  // OSCG begin
  OSCG #( .DIV(6) ) oscillator ( .OSC(c$app_arg_0) );
  // OSCG end

  assign result = topEntity_f_result;

endmodule
leonschoorl commented 10 months ago

Well it no longer duplicates the primitive... The primitive is not instantiated at all and clash has made up a clock out of thin air.

That is in itself is worrying. And seems to happen even on GHC 9.6.2