Skip to content

Commit

Permalink
No inlining let-bound global vars with clock types
Browse files Browse the repository at this point in the history
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
  • Loading branch information
christiaanb committed Nov 18, 2024
1 parent 10f26ff commit b13b96f
Show file tree
Hide file tree
Showing 4 changed files with 27 additions and 1 deletion.
1 change: 1 addition & 0 deletions changelog/2024-11-18T14_59_34+01_00_fix2845
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
FIXED: Clash generates illegal Verilog names [#2845](https://github.com/clash-lang/clash-compiler/issues/2845)
13 changes: 12 additions & 1 deletion clash-lib/src/Clash/Rewrite/WorkFree.hs
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,18 @@ isWorkFreeClockOrResetOrEnable tcm e =
if isClockOrReset tcm eTy || isEnable tcm eTy then
case collectArgs e of
(Prim p,_) -> Just (primName p == Text.showt 'removedArg)
(Var _, []) -> Just True
-- Only local variables with a clock type are work-free. When it is a global
-- variable, it is probably backed by a clock generator, which is definitely
-- not work-free.
--
-- Inlining let-bindings referencing a global variable with a clock type
-- can sometimes lead to the post-normalization flattening stage to generate
-- code that violates the invariants of the netlist generation stage.
-- Especially when this global binder is defined recursively such as when
-- using `tbClockGen`.
-- This then ultimately leads to bad verilog names being generated as
-- reported in: https://github.com/clash-lang/clash-compiler/issues/2845
(Var v, []) -> Just (isLocalId v)
(Data _, [_dom, Left (stripTicks -> Data _)]) -> Just True -- For Enable True/False
(Literal _,_) -> Just True
_ -> Just False
Expand Down
1 change: 1 addition & 0 deletions tests/Main.hs
Original file line number Diff line number Diff line change
Expand Up @@ -635,6 +635,7 @@ runClashTest = defaultMain $ clashTestRoot
, runTest "T2781" def{hdlLoad=[],hdlSim=[],hdlTargets=[VHDL]}
, runTest "T2628" def{hdlTargets=[VHDL], buildTargets=BuildSpecific ["TACacheServerStep"], hdlSim=[]}
, runTest "T2831" def{hdlLoad=[],hdlSim=[],hdlTargets=[VHDL]}
, runTest "T2845" def{hdlSim=[],hdlTargets=[Verilog]}
] <>
if compiledWith == Cabal then
-- This tests fails without environment files present, which are only
Expand Down
13 changes: 13 additions & 0 deletions tests/shouldwork/Issues/T2845.hs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
module T2845 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

0 comments on commit b13b96f

Please sign in to comment.