diff --git a/changelog/2024-11-18T13_43_58+01_00_fix2839 b/changelog/2024-11-18T13_43_58+01_00_fix2839 new file mode 100644 index 0000000000..568318c964 --- /dev/null +++ b/changelog/2024-11-18T13_43_58+01_00_fix2839 @@ -0,0 +1 @@ +FIXED: Clash errored saying it cannot translate a globally recursive function in code that originally only contains let-bound (local) recursion [#2839](https://github.com/clash-lang/clash-compiler/issues/2839). diff --git a/changelog/2024-11-18T14_59_34+01_00_fix2845 b/changelog/2024-11-18T14_59_34+01_00_fix2845 new file mode 100644 index 0000000000..675d46375c --- /dev/null +++ b/changelog/2024-11-18T14_59_34+01_00_fix2845 @@ -0,0 +1 @@ +FIXED: Clash generates illegal Verilog names [#2845](https://github.com/clash-lang/clash-compiler/issues/2845) diff --git a/clash-lib/src/Clash/Normalize/Transformations/Inline.hs b/clash-lib/src/Clash/Normalize/Transformations/Inline.hs index d74a7070dd..5443307eda 100644 --- a/clash-lib/src/Clash/Normalize/Transformations/Inline.hs +++ b/clash-lib/src/Clash/Normalize/Transformations/Inline.hs @@ -120,7 +120,14 @@ bindConstantVar = inlineBinders test True -> return (i `notElemFreeVars` e) _ -> do tcm <- Lens.view tcCache - case isWorkFreeIsh tcm e of + (fn,_) <- Lens.use curFun + -- Don't inline things that perform work, it increases the circuit size. + -- + -- Also don't inline globally recursive calls, it prevents the + -- recToLetRec transformation from transforming global recursion to + -- local recursion. + -- See https://github.com/clash-lang/clash-compiler/issues/2839 + case isWorkFreeIsh tcm e && not (e == Var fn) of True -> Lens.view inlineConstantLimit >>= \case 0 -> return True n -> return (termSize e <= n) diff --git a/clash-lib/src/Clash/Rewrite/WorkFree.hs b/clash-lib/src/Clash/Rewrite/WorkFree.hs index 576fdd6782..48d6417eda 100644 --- a/clash-lib/src/Clash/Rewrite/WorkFree.hs +++ b/clash-lib/src/Clash/Rewrite/WorkFree.hs @@ -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 diff --git a/tests/Main.hs b/tests/Main.hs index 5e24e96d1d..face2877ce 100755 --- a/tests/Main.hs +++ b/tests/Main.hs @@ -635,6 +635,8 @@ 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 "T2839" 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 diff --git a/tests/shouldwork/Issues/T2839.hs b/tests/shouldwork/Issues/T2839.hs new file mode 100644 index 0000000000..6a95bb1e72 --- /dev/null +++ b/tests/shouldwork/Issues/T2839.hs @@ -0,0 +1,12 @@ +module T2839 where + +import Clash.Explicit.Prelude +import Clash.Explicit.Testbench + +topEntity :: + Signal System (Unsigned 8) +topEntity = register clk noReset enableGen 100 0 + where + cntr = register clk noReset enableGen (0 :: Unsigned 8) 0 + done = (== 100) <$> cntr + clk = tbClockGen $ not <$> done diff --git a/tests/shouldwork/Issues/T2845.hs b/tests/shouldwork/Issues/T2845.hs new file mode 100644 index 0000000000..1de593c83f --- /dev/null +++ b/tests/shouldwork/Issues/T2845.hs @@ -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