search

chipsalliance / firrtl

Flexible Intermediate Representation for RTL
https://www.chisel-lang.org/firrtl/
Apache License 2.0
724 stars 176 forks source link

BlackBoxHelper Annotations and Problematic Deduplication #902

Open jackkoenig opened 6 years ago

jackkoenig commented 6 years ago

Type of issue: feature request

If the current behavior is a bug, please provide the steps to reproduce the problem:

FIRRTL annotations generally dedup along with their target modules. However, parameterized blackboxes are different from FIRRTL's perspective if their parameters are different. Thus, BlackBoxHelper annotations targeting these modules do not dedup. This leads to multiple annotations doing the same thing. A common example of this is rocket-chip's plusargsreader. This usually manifests as just writing the same file over and over again, but in various use cases can lead to surprising or undesirable behavior.

See below for desired behavior

What is the use case for changing the behavior?

My main use case involves splitting a FIRRTL design into multiple compilations, which due to multiple BlackBoxResource/Inline annotations pointing to the same Verilog, results in multiple identical Verilog files emitted (thus multiple declarations of the same module).

I'm not really sure how to solve this; we could perhaps have some notion of combining BlackBoxHelper annotations? Multiple BlackBoxResource annotations that refer to the same Verilog file are essentially duplicates, and could be combined into one "super annotation" with multiple target modules. If we decided to adopt this approach, I'm not sure when it should be done, during/after deduplication? Do Annotations need a notion of how to combine themselves? For performance reasons I've though about combining all DontTouchAnnotations into a single, multi-target annotation, but as @azidar as mentioned with the new Target (replacing Named), this can cause problems for deduplication? Maybe it's okay if it's only done after deduplication?

Thoughts @chick, @seldridge?

I can solve this in a hacky way for my use case but I feel like this is a problem that could creep up for others.

Impact: API addition (no impact on existing code)?

Development Phase: request

Other information (e.g. detailed explanation, stacktraces, related issues, suggestions how to fix, links for us to have context, eg. Stack Overflow, gitter, etc)

seldridge commented 6 years ago

Good thoughts, @jackkoenig.

I think this builds a case for a version of FIRRTL that supports parallel compilation and some concept of fork/join. I get into that below, but narrowly, I think the acute problem is BlackBoxSourceHelper being dumb and emitting things multiple times. It seems like an issue of a Transform should know what to do with multiple copies of the same annotation.

Very narrowly, I view annotations as being intentionally dumb. They're metadata that somebody has generated. They shouldn't have any notion of how to combine themselves. They exist and the meaning they provide is only relevant within the scope of a view or a transform. Outside of that, they just exist. This is possibly a pedantic delineation.

Can you provide some more detail on the parallel FIRRTL compilation that you mention?

Onto massive changes...

For a hypothetical parallel FIRRTL there are natural fork and join points and an associated Transform that does this conversion. At a fork, for all modules that are supposed to run in parallel, you create separate circuits for those, convert the forked modules into extmodules, then compilation for top and all extmodules proceeds in parallel. Eventually, this has to hit a join and you convert extmodules to modules based on all available circuits. A natural point for a join like this is in an emitter (or optimizations that require cross-module optimization like DCE, const prop, check comb loops). The emitter then collects all the annotations and the transforms that run in the emitter, like BlackBoxSourceHelper, and those Transforms properly handle multiple copies of the same annotation that exist. This gets particularly ugly because the transform fork/join points may be a function of what annotations exist in the circuit. Some of this may be mitigated if the fork Transform properly splits annotations, e.g., wiring annotations that cross a parallel compilation boundary get converted to wiring annotations that meet at the extmodule boundary. This would have to be some new, abstract method that transforms implement as libraries should be able to provide annotations that cross a fork/join boundary. Naturally, there does not have to be a join and the end result may be separate backend files (a generalization of how the one file per module option works now).

jackkoenig commented 6 years ago

I think you are right @seldridge. For my specific problem, I think I just have to add the intelligence to my splitting or forking to ensure that the right annotations are propagated to the right places. For BlackBoxSourceHelper emitting things multiple times, we should probably just change the transform to stop doing that. Perhaps common patterns will emerge and we can construct utilities, but for this issue I don't think it's that big of a "problem".

As for the notion of fork/joining FIRRTL. I think this is a potentially useful feature. In my case, I only fork the join is in linking the resulting Verilog files together. I think you've describe how to do forking and joining for the FIRRTL circuit, but it is an interesting problem for annotations. I think you've hit the nail on the head with the Wiring Annotations as an example--how can we do this in a general way? In the case of an annotation with multiple targets on either side of the cut, perhaps the information about what to do has to be baked into the annotation, but how do you handle annotations that are implicitly associated with each other (like SourceAnnotation and SinkAnnotation for the WiringTransform)? This seems like a pretty difficult problem to solve in a general way.

seldridge commented 6 years ago

One other approach @jackkoenig, with the Target/instance annotations PR merged, you can start kludging this type of support in by only annotating one instance. It's inelegant, as the general solution is to have the transform do the right thing, but it should work.

For WiringTransform, SourceAnnotation and SinkAnnotation are (I think) kludges that only exist because multi-target annotations are forthcoming. Your observation with the BoringUtils API, that it usually makes sense to annotate via a bore(source, sink), rings true. For the situations where you want to specify stringly-typed sources disjoint from sinks, the disjoint SourceAnnotations and SinkAnnotations are like intermediate/internal/non-serializable annotations. There should be some additional step that converts these to multi-target, serializable annotations.

Positing the existence of multi-target annotations, there then exists something very similar to @azidar's ResolvedAnnotationPaths mix-in. Something like this would clean up a circuit so that it can be cleanly cut (any modules that cross a cut are duplicated). Part of this is already generating a RenameMap that will copy annotations. Renaming already depends on how an annotation defines itself to be duplicated. I think it's logical that then the annotations also define how they can be split if they are multi-target.

It would then seem that a guarantee on a thread-safe Transform is one that implements an execute method that consumes only one multi-target annotation. It then has a restricted notion of execute that sequentially applies that one method on each multi-target annotation individually.