No inlining let-bound global vars with clock types

[christiaanb]

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

Still TODO:

• Write a changelog entry (see changelog/README.md)
• Check copyright notices are up to date in edited files

[DigitalBrains1]

I think we should backport this to 1.8?

Furthermore, it wouldn't surprise me if the reproducer in clash-testsuite that is added here no longer reproduces the issue once issue #2570 is truly fixed. This is just something I realised and wanted to point out.

[leonschoorl]

Are you aware that bindConstantVar already checks for (vars with) local ids?

clash-compiler/clash-lib/src/Clash/Normalize/Transformations/Inline.hs

Lines 118 to 120 in 10f26ff

	test _ (i,stripTicks -> e) = case isLocalVar e of
	-- Don't inline `let x = x in x`, it throws us in an infinite loop
	True -> return (i `notElemFreeVars` e)

I think that means that your (isLocalId v) will be False, except possibly when we recurse inside isWorkFreeIsh.

[leonschoorl]

The local variable distinction feel arbitrary to me.

Given:

clk = clockGen

clkGlobal = clk

f =
 let
   clkLocal = clk
   clkA = clkLocal
   clkB = clkGlobal
 in [...]

As I understand it, you're saying here it is ok for bindConstantVar to replace clkA inside of [...] with clkLocal, but not clkB with clkGlobal.
Is that what you're saying?
And if so, why?
They seem equivalent to me.

[christiaanb]

Yes, that's what I'm saying. Inlining clkA everywhere will not make the circuit f any larger (perform work). Inlining clkB everywhere will make the circuit f larger, because it will have duplicated calls to what will ultimately be clockGen.

[leonschoorl]

Do you remember which invariants?
And are these invariants actually documented anywhere?

[christiaanb]

We cannot have let-expressions appear in the argument position of an application.

[christiaanb]

Are you aware that bindConstantVar already checks for (vars with) local ids?

clash-compiler/clash-lib/src/Clash/Normalize/Transformations/Inline.hs

Lines 118 to 120 in 10f26ff

	test _ (i,stripTicks -> e) = case isLocalVar e of
	-- Don't inline `let x = x in x`, it throws us in an infinite loop
	True -> return (i `notElemFreeVars` e)

I think that means that your (isLocalId v) will be False, except possibly when we recurse inside isWorkFreeIsh.

I am aware. But prior to this commit/PR, the compiler would inline let-bindings binding global variables with a clock type, which is bad because it duplicates work/increases the size of the circuit..

[christiaanb]

I think @leonschoorl comments correctly identify a missed CSE oppertunity in that the current implementation will, for:

clk = clockGen

clkGlobal = clk

f =
 let
   clkLocal = clk
   clkA = clkLocal
   clkB = clkGlobal
 in [...]

not transform f such that ultimately there is only one application/call of the clockGen primitive. While it is most likely desirable in increase sharing in this case, that has never been a guarantee/must of the Clash compiler. That's because CSE is not always desirable as it increases fanout. Decreasing sharing should however be considered a bug, which this PR actually fixes.

That being said, once this PR is merged, we should probably open an "enhancement" issue that Clash is missing a desirable CSE opportunity.

[DigitalBrains1]

I'd like to call your attention to the point that there can ever only be one clock primitive in one domain.¹ Two clock primitives might generate the same frequency, but they are probably not perfectly phase-aligned, making them separate clock domains. So with constructions like clkGlobal = clk, you have to wonder: is this ever actually going to occur in a valid design? Or is this already user error since they are now putting things in the same domain even though they are not synchronised to the same clock? If I want stuff to be in the same clock domain, I pass them all the exact same bind of the clock variable, not clk to one and clkGlobal to another.

¹ The other way around is fine: you could have one clock primitive which outputs multiple clocks.

[DigitalBrains1]

Mergify was not opening a backport PR so I tried to delete and re-add the label. Didn't work; but I remembered you can look at Actions for Mergify stuff. Mergify reports:

error: commit 6f9b7300da535bae01da67eeac99dd82f2cac965 is a merge but no -m option was given.
fatal: cherry-pick failed

and boy something has gone wrong with this PR. Just admire this nice shape:

$ git log --graph --oneline --no-show-signature origin/master
* 8a827e1e2 (origin/master) GitLab CI: Use Stackage Docker image (#2805)
*   47b8fa7c8 Merge pull request #2846 from clash-lang/fix_2845
|\  
| *   6f9b7300d Merge pull request #2844 from clash-lang/fix_2839
| |\  
* | | 8cded4cbe Move block RAM reset value into the `ClearOnReset` constructor of `ResetStrategy`, (#2849)
* | |   25a7bc18f Merge pull request #2844 from clash-lang/fix_2839
|\ \ \  
| |/ /  
|/| /   
| |/    
| * 00722413f Do not inline let-bound recursive calls
* | f13b8534e Add `Distributive` and `Representable` instances to `Vec n` (#2829)
|/  
* 10f26ff16 (gronau/master) Reduce `splitAt` to undefined in illegal contexts (#2837 

I don't know whatever happened here, but yeah, I don't blame Mergify not being able to backport this... D-:

I think PR #2846 was at one point based on PR #2844, and then rebased incorrectly somehow.

[DigitalBrains1]

Commit b13b96f still looks fine, but then it got force-pushed to 6f9b730 and that is a merge commit with contents. It merges master and the commit from #2844, but its tree contains the changes of this PR instead of just being the combination of its parent commits.

[christiaanb]

I have no idea what happened here. My regular way of working is:

• git pull —rebase origin/master
  And then after solving conflicts do a
• git commit
• git rebase —continue
• git push -f
  I guess I should have done a
• git fetch origin
  Before the rebase?

[DigitalBrains1]

pull should already include a fetch. So you shouldn't need to do fetch yourself before a pull.

I also don't know what went wrong, and we actually lost your commit message of this PR in the process. I'm currently handcrafting a backport PR.

[DigitalBrains1]

The intended commit message of this PR is identical to the PR cover letter.