clash-lang / clash-compiler

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

Clash sometimes duplicates clockWizard #2532

Closed leonschoorl closed 1 year ago

leonschoorl commented 1 year ago
import Clash.Explicit.Prelude
import Clash.Cores.Xilinx.VIO
import Clash.Xilinx.ClockGen

topEntity :: Clock System -> Reset System -> Vec 4 (Signal System Bit) -> Vec 4 (Signal System Bit)
topEntity clkIn rst inp = out
 where
  (clk,_) = clockWizard @System @System (SSymbol @"clk_wiz_0") clkIn rst

  out = map (vioProbe ("vio_in":>Nil) ("vio_out":>Nil) 0 clk) inp
  -- out = map (register clk rst enableGen 0) inp

duplicates the clockWizard inside of the map loop:

  // map begin
  genvar i;
  generate
  for (i=0; i < 4; i = i + 1) begin : map
    [...]
    // clockWizard begin
    clk_wiz_0 clockWizard_inst (...);
    // clockWizard end
    [...]
  end
  endgenerate
  // map end

This is might not specify to clockWizard, but I noticed it there because Vivado failed to synthesize this, because it couldn't route one clock port to multiple MMCMs. But even if Vivado were to allow it, MMCMs are a limited resource. And in general clash shouldn't be duplicating things that do work.

If we change the vioProbe for something simpler like a register then clash is able to pull the clockWizard out of the map loop.

I've tried adding Clash.Magic.deDup both around clockWizard and at the usage site of clk, but that doesn't have an effect.

leonschoorl commented 1 year ago

I've had a look at the core that clash gets from GHC. When mapping with vioProbe, GHC seems to do some case transformation where it puts the vioProbe inside of the projection case of the result of the clockWizard:

map
  (case (clockWizard ... clkIn[LocalId] rst[LocalId]) of
     (,) clk ds ->
       vioProbe ... clk[LocalId])
  inp[LocalId]

But when mapping with register for some reason doesn't do that and just maps with let expression for clk with just the straight projection, making it easier for clash to pull it out of the map:

map
  (let
     clk
     = case clockWizard ... clkIn[LocalId] rst[LocalId] of
         (,) clk1 ds ->
           clk1[LocalId]
  in let
       initial
       = fromInteger## 0## 0
  in λi ->
  register# ... clk[LocalId] ...)

  inp[LocalId]