Refactor Annotations

[jackkoenig]

This makes it much easier to write annotations. As an example, check out identity annotation from the tests:

// Annotation used in Firrtl
case class IdentityAnnotation(target: Named, value: String) extends SingleTargetAnnotation[Named] {
  def duplicate(n: Named) = this.copy(target = n)
}
object identify {
  // Convenient constructor for this annotation in Chisel code on Chisel InstanceIds
  def apply(component: InstanceId, value: String): Unit = {
    val anno = LazyAnnotation(() => IdentityAnnotation(component.toNamed, value))
    annotate(anno)
    // We have a transform we want to run whenever something is identified
    annotate(RunTransformAnnotation(classOf[IdentityTransform]))
   }
 }

• Related issue

This is a companion to chipsalliance/firrtl#721

• Type of change

  • Bug report
  • Feature request
  • Other enhancement

• Impact

  • no functional change
  • API addition (no impact on existing code)
  • API modification

• Development Phase

  • proposal
  • implementation

• Release Notes

  • Updates Chisel to new JSON-based Annotation system in Firrtl (see Refactor Annotations firrtl#721).
  • Deprecates ChiselAnnotation in favor of LazyAnnotation.
  • Deprecates Module.annotate in favor of object annotate
  • Adds .toNamed to InstanceId so that we can easily generate Firrtl references to Chisel objects.

[jackkoenig]

TODO

• Add ScalaDoc

[ducky64]

Looks sane, I had some thoughts about naming APIs.

What's the rationale for LazyAnnotation? Is lazy simply an implementation detail that users don't need to care about, or does it affect the behavior of the API?

[ducky64]

For Testers, we do need an API to get the FIRRTL name of a Chisel object. How that should be done (through DataMirror maybe?) and specifics (name qualified up to which level? Top? User-defined root?) are questions though.

[ducky64]

Perhaps not a now thing, but can we relax when this can be called? For example, it should be possible to get the name of any bound wire, assuming it's properly named (instead of T_, which is kind of meaningless anyways) - though we might need to ensure that these names can't be mangled in the future. Going up the module chain also presents difficulties in this case, you never know when an enclosing module might not have a proper name.

[jackkoenig]

I think making it possible to eagerly access the names of things is desirable but it's a non-trivial undertaking. We would need to ensure that querying the name of something does not impact the result. For example:

class NamingTop extends RawModule {
  val w = Wire(UInt(8.W)).suggestName("foo")
  w.getName // ???
  val foo = IO(Input(UInt(8.W)))
  w := foo
}

Currently, w gets the name foo_1 because ports always win. We would need to be sure that calling getName did not accidentally change this.

Doing this with reflection also sounds... dangerous. Perhaps we could make @chiselName disable the reflection-based naming and make eager-naming a feature of using @chiselName?

[ducky64]

I still can't say I'm a fan of an API that depends so much on mutable state where access can be unsafe. Are there other possible APIs, like ChiselAnnotation.toFirrtl taking in a object that can remap a component to a name?

[jackkoenig]

What is the difference between putting the code here in NamedComponent vs. in ChiselAnnotation? In either case, getting a Firrtl Named from a Chisel object relies on mutable state in that object that isn't set until the end of elaboration. This PR is not changing anything about that aspect of Chisel's implementation.

[ducky64]

Lazy seems like "we're abusing this Scala language feature because we're too lazy [heh] to write the plumbing properly" and not balancing the usability / learnability concerns with the implementation effort, probably partially because we're Scala power users and kind of feel that users are also at a similar level.

I think the general convention for lazy evaluation is not that it gives you a different result because of execution order, but it gives you the same result with different performance characteristics. In other words, with few exceptions (for example, infinite lists), lazy is supposed to give the same result as the non-lazy version, which is not the case here.

For this PR in particular, I think Lazy is meant to be a way to get the target name, right? Can we not leave the serialization of the annotation (which is where the name is needed) to the FIRRTL emit stage, passing in a Chisel Data/Module/... reference at the annotation stage? My experience is that the () => ... notation is unnecessarily confusing, and those that don't understand Chisel execution order (which in my opinion is implementation rather than interface) won't understand the motivation and might instead think it's just a poorly designed API. This notation is also not foolproof, as you can pass in a function to an eagerly-evaluated value, and would likely create a very unintuitive error message.

[jackkoenig]

I agree this is kind of an abuse of the term "lazy". It would probably be better to be more explicit and call it something like DelayedAnnotation.

I designed this to require the minimum amount of boiler plate for annotation writers. Using a reference to the Chisel object is possible, but in my opinion it leads to a worse overall API. I'll lay out the two ways I have thought of to do it that way:

1. Duplicate the Annotation

// Consider some Annotation with associated information
case class MyAnnotation(target: ComponentName, info: Map[String, Int]) extends SingleTargetAnno[ComponentName] {
  def duplicate(n: ComponentName) = this.copy(target = n)
}
// I could duplicate the datastructure to also support a Chisel Data
case class MyAnnotation2(target: Data, info: Map[String, Int]) {
  // I would also have to call out what other annotation it should correspond to, 
  // and AFAIK it's impossible to have static type safety that this class corresponds to the other one
  def companionAnno = classOf[MyAnnotation]
}

2. Extend Firrtl's Named to allow "Lazy" or "Delayed" Names

This one is probably better but has some potential issues. I could open up the Named in Firrtl (it's currently sealed) to allow Chisel types to extend it. If we want any type safety, we'd have BaseModule extend ModuleName and Data extends ComponentName and have some notion of a Lazy or Delayed Name. This then means that whenever the user uses that annotation, they have to check that the name is actually known. This would add boiler plate to use. Alternatively, I could overwrite the unapply methods on ComponentName and ModuleName and put the dynamic check there.

My main issue with 2. is that there's a bit of magic going on behind the scenes. I suspect that people could run into subtle issues trying to match on Chisel types in Firrtl transformations (which would actually work so long as you didn't serialize between Chisel and Firrtl!) The proposal on this PR is the least magical and most explicit way to do it. Sure, () => is confusing to new users, but if someone is writing custom annotations, they're most certainly also writing custom transformations which is a whole larger bag of worms then learning what a function as an argument is.

[jackkoenig]

Updated this with a more explicit ChiselAnnotation trait that should be extended in Chisel libraries that use Annotations. You can still create anonymous ones if you don't want the boiler plate, but they do provide a reasonable API for corresponding Annotations in Chisel-land and Firrtl-land.

I also add RunFirrtlTransform to preserve behavior of Driver.execute instantiating Firrtl transforms associated with a given Annotation.

[ducky64]

Two comments

[ducky64]

Why is annotate called twice?

[jackkoenig]

Mistakenly left an old API, fixed

[ducky64]

I still can't say I'm a fan of an API that depends so much on mutable state where access can be unsafe. Are there other possible APIs, like ChiselAnnotation.toFirrtl taking in a object that can remap a component to a name?

[ducky64]

looks good, current understanding is that order-dependent stuff will fail noisily if invoked improperly.

[jackkoenig]

retest this please

[jackkoenig]

Can any one reproduce the failure I'm seeing on Jenkins (DontTouchSpec failing)? I can't seem to reproduce it.

[ucbjrl]

[info] DontTouchSpec: [info] Dead code [info] - should be removed by default *** FAILED *** [info] "module HasDeadCodeChild( // @[:@3.2] [info] input [31:0] io_a, // @[:@6.4] [info] output [31:0] io_b, // @[:@6.4] [info] output [31:0] io_c_0, // @[:@6.4] [info] output [31:0] io_c_1 // @[:@6.4] [info] ); [info] assign io_b = io_a; [info] assign io_c_0 = 32'h0; [info] assign io_c_1 = 32'h0; [info] endmodule [info] module HasDeadCode( // @[:@12.2] [info] input clock, // @[:@13.4] [info] input reset, // @[:@14.4] [info] input [31:0] io_a, // @[:@15.4] [info] output [31:0] io_b // @[:@15.4] [info] ); [info] wire [31:0] inst_io_a; // @[DontTouchSpec.scala 27:20:@17.4] [info] wire [31:0] inst_io_b; // @[DontTouchSpec.scala 27:20:@17.4] [info] wire [31:0] inst_io_c_0; // @[DontTouchSpec.scala 27:20:@17.4] [info] wire [31:0] inst_io_c_1; // @[DontTouchSpec.scala 27:20:@17.4] [info] wire [32:0] _T_10; // @[DontTouchSpec.scala 30:28:@22.4] [info] wire [31:0] _T_11; // @[DontTouchSpec.scala 30:28:@23.4] [info] wire [31:0] dead; // @[:@24.4] [info] HasDeadCodeChild inst ( // @[DontTouchSpec.scala 27:20:@17.4] [info] .io_a(inst_io_a), [info] .io_b(inst_io_b), [info] .io_c_0(inst_io_c_0), [info] .io_c_1(inst_io_c_1) [info] ); [info] assign _T_10 = io_a + 32'h1; // @[DontTouchSpec.scala 30:28:@22.4 ] [info] assign _T_11 = _T_10[31:0]; // @[DontTouchSpec.scala 30:28:@23.4] [info] assign dead = _T_11; // @[:@24.4] [info] assign io_b = inst_io_b; [info] assign inst_io_a = io_a; [info] endmodule [info] " included substring "io_c_0" (DontTouchSpec.scala:45) [info] - should NOT be removed if marked dontTouch [info] Dont touch [info] - should only work on bound hardware

…

On Wed, Feb 28, 2018 at 12:03 PM, Jack Koenig ***@***.***> wrote: Can any one reproduce the failure I'm seeing on Jenkins (DontTouchSpec failing)? I can't seem to reproduce it. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#767 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGh1wNuwmVO7YYgOs7ZpuYdrp4NywW6eks5tZbETgaJpZM4RvCx2> .

[jackkoenig]

Well, ensuring the tests each had their own test directory fixed the problem 🤷‍♀️. Anyway for rocket-chip bumping reasons, I'm going to merge #788 first. Then I'll merge this.