chipsalliance / chipsalliance/chisel
Definition/Instance and doNotDedup Interaction
- Dominant language
- Scala
- Stars
- 4.8k
- Forks
- 658
- Avg merge
- 18h 59m
- Merged PRs (30d)
- 14
Description
Presently, any use of `NoDedupAnnotation` or it's Chisel-level API, `doNotDedup`, will be silently ignored by a Chisel compilation.
Consider the following circuit. Scastie snippet [here](https://scastie.scala-lang.org/apTbulfdSza3wrq7fpwaqg):
```scala
import chisel3._
import chisel3.experimental.hierarchy.{
instantiable,
public,
Definition,
Instance
}
import chisel3.stage.ChiselStage
@instantiable
class Bar extends Module {
@public val in = IO(Input(Bool()))
@public val out = IO(Output(Bool()))
out := in
chisel3.experimental.doNotDedup(this) // <-- This doesn't matter in D/I land
}
class Foo extends Module {
val in = IO(Input(Bool()))
val out = IO(Output(Bool()))
private val barDef = Definition(new Bar)
val bar1, bar2 = Instance(barDef)
bar1.in := in
bar2.in := in
out := bar1.out ^ bar2.out
}
```
This is a main module, `Foo`, that instantiates two copies of the same definition of `Bar`. However, `Bar` has internally declared itself as `doNotDedup`. Then end result of this elaboration (and compilation with a FIRRTL compiler) is that `Bar` is deduplicated.
I'm not sure how to handle this... There are only a limited number of places where Chisel uses this API (including a handful of SiFive-internal utilities as well as) `InlineInstances` and `BoringUtils`.
What I think is going on here is that `doNotDedup` is a relic of when annotations were limited to being `Named` where the specificity of `Target` to specify a path to a _specific_ instance was not present. In that world, annotations used to block deduplication because specificity was lost by allowing un-annotated and annotated things to merge. We eventually relaxed that, but then we needed a way to keep legacy annotations working (whose transforms required that they were operating on unique instances).
What we could probably do here is to work towards full removal of `Named` (migrating to `Target` for annotations which don't currently use it) and updating downstream transforms to understand non-local `Target`s would enable removing this API and allowing people to D/I their designs however they want. Alternatively, FIRRTL compilers may need to change their interpretation of `NoDedupAnnotation` from "do not deduplicate me" to "make sure I am unique".
Contributor guide
Research direction
Start with the linked Scastie example and trace how Definition/Instance handles Bar's doNotDedup and NoDedupAnnotation during Chisel and FIRRTL compilation. Review the existing uses of this API, including InlineInstances and BoringUtils, and determine whether the intended behavior is preserved. Done requires an agreed semantics and regression coverage for deduplication.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- scala
- Domain
- compilers
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100