chipsalliance
D/I Question -- contextual information for Instances
Definition/Instance API allows us to specify a Module once then stamp down copies of it. This works well from Chisel -> FIRRTL, but it's unclear what the mental model is as it goes through FIRRTL. Currently FIRRTL (specifically SFC) can modify Modules/Instances in the following ways:
- [A] Reset Inference -- change the types of flops inside based on what the reset of an instance is connected to
- [B] Constant Propagation -- eliminate certain inputs, outputs, or internal logic based on what is connected to the instance
- [C] Width inference -- decide the width of internal signals, registers, I/Os based on what is connected to the instance
- [D] Arbitrary annotations -- change pretty much anything based on what annotations are applied to the Instance
For D/I API to be useful, we need to clearly define what is allowed/expected behavior for each of these cases.
My observations are:
[A] Reset Inference
SFC seems to check that all of the Instances of the Definition will have the same inferred reset type: https://scastie.scala-lang.org/O1QxHS4nQ8aITDFCB7suhA
The Definition's reset type is ignored.
https://scastie.scala-lang.org/ORaed9rCRRORAapWYiq3wg and https://scastie.scala-lang.org/kvx1kLUBS5aHtPHIrmVyQA
[B] Constant Prop
I think that constant prop will happen if it can be constant-propped for all instances, and will not happen otherwise. TODO Scastie example
[C] Width Inference
Seems to take the maximum width? https://scastie.scala-lang.org/5Bzl0WCDQ2ewUJ65Z8Hv0Q
[D] Arbitrary Instance Annotations
Not sure yet
Type of issue: feature request
Impact: unknown
Development Phase: request
Other information
If the current behavior is a bug, please provide the steps to reproduce the problem: See Scasties Above
What is the current behavior? See above
What is the expected behavior?
Not sure. My request is that we make sure we are clear on what the expected behavior is.
Please tell us about your environment: - version: 3.5.1
What is the use case for changing the behavior?
More useful Definition/Instance API
B should fall out naturally for any classic const-prop algorithm (as these can already handle multiply instantiated things).
C should fall out from gather all constraints through every instantiation, which means a solution must be valid at every instance. It also means, I think, that inference has a much higher chance of failing now.
A is a annoying problem in the language (resets being tied to global analysis).
D unfortunately requires interpreting every possible annotation in light of multiple instantiation, which is a challenging, maybe impossible task (out of tree annotations).
Note that for [A], what is actually going on is that withReset has no effect on a Definition. withReset creates a node cast to async reset. Definitions doesn't actually create hardware, so the async reset node is dead and will be deleted later. All that matters is what happens on the Instance because that is where hardware is actually connected.
One thing to add - there is a distinction between what is a possible implementation (e.g. supporting width inference by gathering instance constraints) versus what is a desired semantic (do we want instantiating a new instance to have the side-effect of affecting all other instance's hardware?).
For each item, we should brainstorm both for the implementation and desired semantic.
Regarding D, we only need to care about instance annotations which are actually consumed by an RTL-changing transform. Presumably, these are all known in MFC, and thus can be scoped. Adding new annotations shouldn't be a today issue because we would also need to add a new transform to do the RTL change, and thus we can decide the semantics for that transform when we add it.
A, B, and C should all maintain the single Definition, thus it must give the same result for all of them. Thus B and C are semantically correct.
The weirdness in A is just a bug, this is a known issue (although looks like I never filed it), that Chisel defaults to Bool for the top module and that D/I is treating Definitions as top modules, rather, Chisel should change to allow inferring even the top-level reset.
The semantics of D are the same as they are for any annotation, it will probably require more careful handling of when annotations may refer to instances, but I don't think there's anything fundamentally different from how this works with Dedup.
(do we want instantiating a new instance to have the side-effect of affecting all other instance's hardware?).
Exactly -- this makes me think we need two kinds of Instances, if this is the kind of thing that can happen today. Without this guarantee that Instances can't effect eachother (basically changing the Definition) this is a less useful API.
The weirdness in A is just a bug, this is a known issue (although looks like I never filed it), that Chisel defaults to Bool for the top module and that D/I is treating Definitions as top modules, rather, Chisel should change to allow inferring even the top-level reset.
Note: this isn't quite right. The CHIRRTL produced has an abstract reset for the child. The problem is that Definitions are unaffected by withReset because this is an API that connects resets (and there is nothing to connect for a definition).
A, B, and C should all maintain the single Definition, thus it must give the same result for all of them. Thus B and C are semantically correct.
The issue with this is that we currently have no way of doing the following:
Build a design with Definition/Instances. Instances may evaluate slightly differently given what else is in the design. We seem to be in agreement that is acceptable today.
Now I want to grab an Instance or Definition from that circuit, then build a TB around it. I don't want the TB to participate in the evaluation from before, but I do want to "do the right thing" -- ensure that widths propagate the same, I drive the correct reset, etc, so that my TB is useful. A few possible approaches:
-
Guess-and-verify -- TB writer makes their best guess (or gets side-channel information) about the reset type, widths, and any constant/nonexistent inputs, then makes sure they connect up their Instance the same way. If they guess wrong, somewhere in the flow they get an error similar to what happens to the resets today, "You tried to drive this with a UInt of width 3, but actually the Definition/Instances already had width 4, go fix your connections.").
-
Ask the truth -- TB writer has a way, either explicitly ("val tb_reset = Wire(new myInstance.reset.cloneResetType())") or implicitly/connection-propogated (
val tb_reset:Reset := instance.reset) of querying what the Instance already knows, without influencing it, and then building their TB code around it.
Bottom line: currently we have no Chisel API for "don't influence this through FIRRTL stage", aside from Black-Boxing
It also seems strange that the behavior is different today from [A] vs [B] and [C], as they feel like the exact same problem to me.
Note: this isn't quite right. The CHIRRTL produced has an abstract reset for the child. The problem is that Definitions are unaffected by withReset because this is an API that connects resets (and there is nothing to connect for a definition).
@seldridge I stand corrected. I misread the original message, yes a withReset on the Definition is ignored because Definitions are fundamentally "out-of-tree" and withReset only affects things in tree. If we can find a way to avoid user confusion here, that would be great.
So yeah A, B, C and are all doing the right thing. While it is true that A requires more inference to run than B and C, they are all fundamentally global analyses so the same problem exists for all of them for what Megan wants to do, which brings me to...
@mwachs5
Bottom line: currently we have no Chisel API for "don't influence this through FIRRTL stage", aside from Black-Boxing
To echo my inner @seldridge, fundamentally, we can think about each of these things (reset type, width, and the existence or non-existence of various ports) as parameters. So by analogy, pretend we're wanting to test HashMap<K, V> (using Rust as example because rust monomorphizes generics). If you're wanting to test HashMap<String, Gobbledygook> based on the usage of HashMap<K, V> in some program, you're either going to need to just manually specify that you want to test HashMap<String, Gobbledygook> or you're going to need to compile the other program and use reflection to determine the value of the type parameters.
So that gives us two approaches for the user to solve the problem:
- "Parameterize" the
Definitionin the same way. - Provide a reflection API such that the user can compile the
Definitionsufficiently to reflectively determine the values of the parameters.
Your point @mwachs5 (well taken) is that we need APIs in Chisel to help with these approaches. At a minimum, we need a simpler way to "parameterize" a Definition ("parameterize" in quotes because these aren't handled as true parameters, they're like FIRRTL built-in "generics"). We also should think about what reflection would look like here, we need a way to load pre-compiled Definitions with enough information to reflect on how it was compiled, or we need need a way to say in Chisel "compile this Definition enough to reflect on it".
@azidar has added an option 3 which is:
- Disallow uses of A, B, and C on the interfaces of Definitions.
I would tweak that slightly to mention that this often means hoisting these "implicit generics" to become explicit parameters in the Scala.
After discussing it some, it seems like the thoughts are:
-
[B]and[C]should be documented for users along the lines of "creating Instances of a Definition where constant prop/DCE/width inference will happen across the interfaces should be seen as a constraint solving problem -- the Definition will resolve to something that satisfies the constraints of all Instances. Therefore adding a new Instance in a different context can result in changes to the Definition for all instances". In theory there could be places where no solution is possible, in practice I can't think of any today. But if there hypothetically were some ("I want a width < 10 for io.in" and "I want a width > 20 for io.in") then this would fail during width inference/constant prop/dce with an error message to user. - Users who don't like the above can prevent constant prop across their Definition (with DontTouch) and width inference (by not using any inferred width signals).
-
[A]is basically the same as[B]and[C]except there is no solution to the constraint "I want a sync reset" and "I want an async reset" so we fail, as we would in the hypothetical case above. Users who don't like this can make their Reset type explicit by using RequiresSyncReset/RequiresAsyncReset/only using specific reset types for their module interfaces.