chipsalliance / firrtl

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

Inline Compilation Annotation (JSON function serialization) #847

Open seldridge opened 6 years ago

seldridge commented 6 years ago

Type of issue: bug report | feature request | other enhancement

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

I'm uncertain if this is something that even makes sense, but it comes up for inline compilation. I'd like to have a clean way to serialize a function that I want to pass to Chisel's Driver to create a circuit. This parameter is, naturally, a function. However, serialization is really only supposed to work for data and I'm trying to communicate information which is not data. It's unclear to me if this is something that should be supported by json4s, by our serializers, or if there's a good pattern for doing things like this.

This small test case shows an example. I'm trying to serialize the () => Module argument for Chisel's Driver.

case class GeneratorAnnotation(gen: () => Module) extends NoTargetAnnotation

class MyTest extends ChiselFlatSpec {

  "GeneratorAnnotation" should "work" in {
    class MyModule extends chisel3.Module { val io = IO(new Bundle{}) }
    val x = GeneratorAnnotation(() => new MyModule)
    val json = firrtl.annotations.JsonProtocol.serialize(Seq(x))
  }

This causes the JSON serialization to fail with a somewhat cryptic error message that doesn't provide much indication of what was wrong:

[info] GeneratorAnnotation
[info] - should serialize *** FAILED ***
[info]   org.json4s.package$MappingException: Classes defined in method bodies are not supported

I can get around this by passing the class and then using reflection to create a new instance, however, this gets complicated if the Module constructor takes parameters or if there are multiple constructors, I'm trying to get at a companion object, etc. This is possibly doable with serialization of the arguments, but that all just seems overly (though perhaps necessarily) complicated. (And this is how I used to do this...)

What is the use case for changing the behavior?

This seems to come up frequently as parameters with signature () => T are used in a lot of places.

Impact: unknown

Development Phase: request

jackkoenig commented 6 years ago

Yeah this is a difficult problem. AFAIK magical [de]serialization libraries like json4s don't support functions. I suspect it is possible but it probably involves some reflection hackery where the class loader of the function/inner class/etc. needs to be serialized as well.

The interaction with type parameters is a-whole-nother bag of worms, but the fact that we can do it for [T <: Named] suggests it could be done at least for specific annotations for a concrete-subclass. I don't know if there's an automatic way to do it generically. To generically deserialize Named we had to provide an implementation for the abstract class: https://github.com/freechipsproject/firrtl/blob/ebb6847e9d01b424424ae11a0067448a4094e46d/src/main/scala/firrtl/annotations/JsonProtocol.scala#L21

seldridge commented 6 years ago

Reflection hackery seems to be the correct way to go.

However, it occurred to me that so long as the argument is actually () => Module, then there's no specific reason to store this in the annotation. It's likely more sensible to elaborate (and possibly convert) into a saner ChiselCircuitAnnotation or FirrtlCircuitAnnotation. The former is somewhat problematic as a Chisel circuit isn't capable of being serialized (as far as I can tell).

I'm using a rough version of this in the Annotations as Options refactor for Chisel3. If we do not care about a command line () => Module, then this approach will work fully. If we do want a --dut command line argument that supports arbitrary parameters, then a more complicated approach would be needed. Similarly, if the lambda has parameters, then a more complicated approach is needed.

chick commented 6 years ago

I think this is unsolvable for now. But it's not that big a deal because we currently can't serialize this anyway and our unit test framework works, it's a bit ugly here and there but it works. I'd vote for creating this unserializable annotation for the generator and proceed from there.