Signal portals

[rowanG077]

Motivation

In Clash designs it is often inconvenient to thread a signal from a deeply nested component back up, or down, to where it is consumed. The Clash ILA is the clearest example: you typically define the ILA near the top-level IO boundary, where the UART pins live. Connecting an internal signal to it requires manually plumbing that signal up through every intermediate module boundary. It is not difficult to do, but it is tedious and invasive, you change signatures and instantiation sites that have nothing to do with what you are debugging.

Signal portals are a proposed escape hatch. Mark a signal at its origin with a name, and retrieve it by that name from anywhere in the hierarchy, with Clash automatically adding the necessary ports between them during HDL generation.

Proposed API

portalSource :: BitPack a => String -> Signal dom a -> Signal dom a
portalSink :: BitPack a => String -> Signal dom a                                        
resetPortalMap :: IO ()

portalSource is a pass-through, it returns the same signal it receives, but marks it in the design so it can be retrieved elsewhere by name. portalSink retrieves that signal. The name must be a compile-time string literal for synthesis to work. resetPortalMap is there to be able to do multiple simulations runs, more on that later.

HDL generation

After netlist generation a dedicated pass could walk the component hierarchy and rewrites matching portal sources and sinks into ordinary input and output ports, propagating them upward and downward through intermediate instantiations as needed. Portals that are entirely contained within a single component subtree do not produce any ports outside that subtree. Unused sources produce a warning (or error under -Werror).

Simulation

Simulation is where this gets more hairy. There is no way to implement this without breaking Haskell's referential transparency.

Concretely:

1. Global mutable state. portalSource and portalSink communicate through a process-wide map which is accessed using unsafePerformIO & friends. This could perhaps at least be made thread local.

2. Evaluation-order sensitivity. portalSink would defers its map read to first-sample time. If the source has already been registered by then this works. If the source has not been registered yet, the only thing we can do is error out as there is no way to connect these two independent lazy thunks. I doubt this is fixable, but if anyone has an idea then I'm all ears.

3. Loss of (compile time)type safety. The proposed API would route through a pack, unpack path which means during simulation you can only check if the width of the source and sink agree. This check could be expanded to full simulation time type safety using TypeRep. This is not a constraint that clash circuits typically use though. So you'd need to add it to all type signatures as well. In either case you lose compile time type checking. During HDL generation you can check for equivalent HWType so this is less of an issue.

4. Re-run hygiene. Each portal name is registered once per process(or per thread) lifetime. To run a simulation a second time in the same process (e.g. in a test suite), resetPortalMap must be called first. This quite something to burden on users.

Questions

• Do we even want to offer this API? It almost feels like an anti-feature to me. But I also recognize that it would be extremely handy in a pinch. We could give them scary names like unsafePortal*.

• Should portals be restricted to synthesis-only (i.e. error in simulation rather than attempting the unsafePerformIO route)? That would simplify the story considerably at the cost of not being able to simulate designs that use portals.

• Are there use cases beyond debugging instrumentation (ILA, dumpVCD-style probes) that would justify the added complexity?

[martijnbastiaan]

I can see the netlist generation part happening, though you need to account for the fact that Synthesize boundaries rely on their ports being known up front (just based on their type signature) so they can be generated in parallel:

clash-compiler/clash-lib/src/Clash/Driver.hs

Lines 346 to 350 in 9aacba0

	maybeMapConcurrently_
	| opt_concurrentTopEntities opts = mapConcurrently_
	| otherwise = mapM_
	maybeMapConcurrently_ (go compNames idSet edamFiles ioLock deps topEntityMap) tes

As for the simulation stuff I'm much more hesitant. Perhaps it makes more sense to iterate on the dumpVCD stuff more before we offer yet another API with unsafePerformIO quirks. One thing that stood out to me was:

This could perhaps at least be made thread local.

This would kill any use of parMap, which I doubt is a good idea. How about requiring functions like these to take an implicit parameter ?traces :: TraceMap? That would contain an IO-locked map that gets filled in with every sampleN (or something like that). We could even hide it in (hidden) clocks or the like. That infrastructure could then be reused by dumpVcd and ILAs alike.

[lmbollen]

I'm also inclined to think an implicit parameter would be the way to go to have a "local" store for whatever.
I also think this would be useful to replace traceMap#, but that's another discussion

Perhaps it would not be a bad idea to have a single implicit parameter to do these kinds of things.

I'm not sure if using Clock is a good idea, since you can have many clocks in one design.
On the other hand, any signal has to have some reference clock and it would prevent us from having to introduce a new constraint.

[martijnbastiaan]

Why would that kill parMap?

You might want to use this within a design to speed up simulation. I.e., maybe you need the output of 3 RISC-V cores, then it might make sense to get them using parMap. However, that would mean they'd run in different threads, and would therefore be added to a different part of the tracemap.

[rowanG077]

If you have to thread a full on new constraint through an entire design it gets nearly as bad as just routing these signals. This is also why I chose BitPack over TypeRep, even though TypeRep is strictly better in terms of what it gives you.

I don't think it is bad to store it in a Clock, at least for signal portals. It basically stops you from using portals as a CDC mechanism, which I think is fine.

You might want to use this within a design to speed up simulation. I.e., maybe you need the output of 3 RISC-V cores, then it might make sense to get them using parMap. However, that would mean they'd run in different threads, and would therefore be added to a different part of the tracemap.

If you run 3 different parallel simulations it makes sense to me they wouldn't share a traceMap just like they wouldn't share anything else. I think even that you don't want to share traceMaps at all in that case.

[rowanG077]

This also brings up another point: Multiple instantiations of the same component. This is a problem during synthesis as well as simulation. But you might sidestep it by simply disallowing it.

[martijnbastiaan]

If you run 3 different parallel simulations it makes sense to me they wouldn't share a traceMap just like they wouldn't share anything else.

What I meant is in a single design. With parMap you can tell the Haskell runtime to evaluate different thunks in parallel. Optimizations like this for a single simulation would break thread-aware tracing, even though they're meant to be transparent. I don't think this is very prevalent, but you know :)

[kleinreact]

Do we even want to offer this API? It almost feels like an anti-feature to me. But I also recognize that it would be extremely handy in a pinch. We could give them scary names like unsafePortal*.

While I clearly share the desire to have such feature for debugging and convenience purposes, I am against it in the given form, mainly for two reasons:

• It completely breaks the semantics of Signal, which makes it impossible to argue about the resulting code in any formal or well-defined context later on. Moreover, once introduced, we need to expect it being used at any possible point in the user's code. Major technical challenges/limitations that would follow from such addition have already been highlighted quite well in the previous comment's discussion, so I won't explicitly restate all of them here again.
• I only see a real benefit in such feature for debugging purposes, which is expected to require some additional infrastructure that needs to be added independently of the actual main design. However, in the current proposal the feature does not really separate such particular feature from any other potential dirty hacks. Therefore, I would propose to better separate the feature or move it to a later stage in the design instead, as for example done by the GateMate ILA, which is completely agnostic of the primary design and just added afterwards, if needed.

you typically define the ILA near the top-level IO boundary, where the UART pins live.

This clearly is the major use case, where existing solutions are mostly inconvenient with Clash designs atm. Not only if you like to use an ILA, but also if you like to use an external logic sniffer, like a Saleae or an Analog/Digital Discovery device (does anybody call that an ELA 🤔).

In that regard, my proposal would be to instead limit the scope of your portal to unused/new ports in the design. So if we only can "portal" some of the internal signals to some additional output ports that are "silently" added to the top entity, e.g., are not part of the default topEntity type signature, but only of the generated HDL output, then it is easy to interface with those at a later stage. In case of the Clash ILA, you then simply could create two top entities. One with the original design and the additional ports, and one which adds the ILA and other debugging infrastructure. You only connect them then later on, if needed for debugging or other testing purposes. That should also avoid most of the issues discussed before.

My main point is that if we like to add such a feature, then it should be embedded into a more restrictive scope than currently proposed. That scope should make it clear to only be applicable in a debugging context, and it should be always considered as a drop-in extensions, rather than a fundamental feature, that could end up being a crucial interface as part of the final design.

[martijnbastiaan]

Would it help if it's a CPP/Cabal flag? E.g., -fsignal-portals (or however you do that in Cabal). I.e., it is completely patched out if you don't set that flag similarish to how profiling works in GHC.

[kleinreact]

Yeah, this is how I think about it from a usage perspective, while I would prefer a proper Clash flag other than some external hacky tool's foo.

I just like the feature to make it perfectly clear not to be used in a productive context. So you can use it for some specific analysis purposes, but you should never even think about using it in your actual design.