chipsalliance / chipsalliance/chisel

Definition/Instance and doNotDedup Interaction

Open
#2,409 3 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.