Merge pull request #2844 from clash-lang/fix_2839

Do not inline let-bound recursive calls

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).

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)

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)

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

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

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

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