From b13b96fa791cd95ebe4b8b34035da8d983381d42 Mon Sep 17 00:00:00 2001
From: Christiaan Baaij <christiaan.baaij@gmail.com>
Date: Mon, 18 Nov 2024 15:00:29 +0100
Subject: [PATCH] No inlining let-bound global vars with clock types

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
---
 changelog/2024-11-18T14_59_34+01_00_fix2845 |  1 +
 clash-lib/src/Clash/Rewrite/WorkFree.hs     | 13 ++++++++++++-
 tests/Main.hs                               |  1 +
 tests/shouldwork/Issues/T2845.hs            | 13 +++++++++++++
 4 files changed, 27 insertions(+), 1 deletion(-)
 create mode 100644 changelog/2024-11-18T14_59_34+01_00_fix2845
 create mode 100644 tests/shouldwork/Issues/T2845.hs

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/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..fff115e0a8 100755
--- a/tests/Main.hs
+++ b/tests/Main.hs
@@ -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
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