Add concurrent normalization

[martijnbastiaan]

This adds concurrent normalization by wrapping all state (fields) in MVars. Earlier patches used STM, but this didn't yield a performance benefit due to excessive contention/rollbacks. Explicit locks have all the usual drawbacks of potential deadlocks, which is why TracedMVar has been added. This can be enabled by CLASH_DEBUG_MVAR. Earlier tests on the bittide project yielded a 300% performance increase.

The feature is disabled by default, but can be enabled using -fclash-concurrent-normalization. It is enabled in the clash-testsuite.

Still TODO:

• Write a changelog entry (see changelog/README.md)
• Check copyright notices are up to date in edited files
• Rebase on master
• Actually listen to -fclash-concurrent-normalization
• Check on bittide-hardware
• Investigate Hit specialization limit 20 on function `Clash.Explicit.Testbench.outputVerifierWith[454934]'.. This can be triggered semi-reliably by executing cabal run clash-testsuite -- -j8 -p clash --hide-successes -p XilinxDDR.

[martijnbastiaan]

@rowanG077 I let Claude have a look at the issue. The reasoning it came up with sounds reasonable to me, but I haven't checked it in detail so please be careful :). The test suite passes locally. I'll test it on bittide-hardware too

---

Problem

The commit "Add concurrent normalization" introduced a race condition in the specialization cache. The specialisationCache and specialisationHistory were converted from pure state (sequential access via lens (%=)) to MVar-based shared state, but the cache-check → history-increment → cache-insert sequence was not atomic.

With concurrent normalization, multiple threads normalizing different entities (e.g., testBenchGS, testBenchGA, testBenchUS, testBenchUA) would all try to specialize the same function (ignoreFor) simultaneously. Each thread would:

1. Check the cache → empty
2. Increment the specialization history counter
3. Do the specialization work
4. Insert into the cache

Since steps 1-4 weren't atomic, N threads would each increment the counter for the same logical specialization, causing the counter to reach N× the expected value and exceeding the specialization limit of 20.

Fix

Two changes in Specialize.hs

1. Changed the cache type from Map key Id to Map key (MVar Id) in Types.hs, using a "future" pattern (same as normalizeTopLvlBndr already uses for the normalized cache).

2. Atomic cache reservation: When checking the cache, we atomically either find an existing entry (and wait on it if it's still being computed) or insert an empty MVar as a placeholder. This ensures only one thread does each specialization, and the history counter is only incremented once per unique specialization.

[rowanG077]

Ah that's close to what my hunch was. Interesting that it can do that. I will check in the code whether it's actually true. If true I'm surprised how good it is.

[martijnbastiaan]

Modern Claude with VSCode / terminal integration is very close to magic.

[martijnbastiaan]

On bittide this does speed up Clash, but it trades it for high memory usage (5.5 GB -> 21 GB). Reducing the thread count from -N8 to -N2 reduces memory usage to 17 GB.

[rowanG077]

And this was not that much of an improvement right and requiring manual OPAQUE on large components?

[martijnbastiaan]

With OPAQUEs it is a 33% improvement. Nothing to sneeze at for 18m runs 🫠..

[martijnbastiaan]

It seems to be fundamental to how Clash normalization works: due to its lifting of binders and specialization caches Terms just stay around for a looong time. I don't think there is much incremental we can do, though I'm open to ideas.

[rowanG077]

Oh that is MUCH more significant than I thought

[rowanG077]

I fixed a deadlock that occurs when a thread is waiting on a specialization result of another thread that never completed.

I also added listening to -fclash-concurrent-normalization

The actual implementation looks fine to me now.

[martijnbastiaan]

Cool, I'll have a look soon.

In retrospect I should have made TracedMVar for qualified use, instead of trying to be consistent with the existing MVar module. What do you think?

[rowanG077]

In retrospect I should have made TracedMVar for qualified use, instead of trying to be consistent with the existing MVar module. What do you think?

I don't have really strong feelings either way. I do like that the API matches the standard MVar which signals to developers that this is just an MVar under the hood. So if you ask me I would keep it like it is currently.