-
Notifications
You must be signed in to change notification settings - Fork 315
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FIRRTL][InferWidths] When there is no unique minimal solution for width constraints, what should be the width? #6924
Comments
The error is saying that there isn't enough information to choose a width. In this case, as you identify, this is a system of linear inequalities and there are multiple solutions that are possible here. (A FIRRTL compiler won't choose one possible solution.) Either the width of It's thereby illegal FIRRTL. While the width inference algorithm is internally expressed as a (very simple) set of constraints that are solved, it is intended to be (and practically after #6917) purely feedforward. |
okay, if it is a case of illegal program when there is no unique solution for constraint solver, is there a specification for all compilation passes that defines illegal cases? |
I am not sure what is the exact problem here. If the constraints are
and implicitly also
Then I think (x1,x2) = (1,1) is a solution. Why does the constraint solver not find this solution? |
Minor correction: the implicit constraint is really The simplistic reason why it doesn't find the That said, this algorithm is generally trying to be feedforward. Known widths are used to determine unknown widths based on the width propagation rules of primitive operations. This generally matches how designers are using uninferred widths (in the limited uses where they are using uninferred widths). The example here is different from this use case in that the request is about choosing a minimum width given only unknown widths. To help narrow the discussion, the original reported circuit can be reduced to the following with all primitive operations removed: module Foo :
input clock : Clock
output io : UInt<10>
reg a1 : UInt, clock
reg a2 : UInt, clock
a1 <= a2
a2 <= a1
io <= a1 Here, the inequalities are So, I can see this going in two directions:
(2) may be better and is more aligned with changes we've been wanting to make to width inference to make it simpler. The current constraint solver is insanely expensive for what it is doing. It builds up a complete shadow IR of constraints and then solves this. Most designers have empirically shied away using unknown widths because they're unpredictable. In the designs that we have, width inference is both slow, a memory hog, and not actually inferring that many widths because these are so rarely used. There have then been discussions about making width inference an incredibly simple feedforward, iterative algorithm that avoids framing this as a constraint problem. I anticipate that this algorithm also won't work for the example above, though that likely aligns with how designers are using Chisel and unknown widths. |
TL;DR: I recommend to simplify width inference to handle only acyclic dependencies. We are also thinking about an implementation of the width inference algorithm. If the width dependency graph is acyclic—one might interpret “simple feedforward” as this—it is sufficient to infer widths by calculating them in the topological order of the graph. For us, that would be the most convenient solution, and it might be sufficient for the (few) uses where unspecified widths are used in practice. However, if one allows cycles like the ones in these examples, a more complex algorithm is needed. A legitimate use of width-inference cycles in designs might be a counter register in a reusable module, with something like The original example of wky17 still seems to exhibit a bug in the width inference constraint solver. It contains a combinatorial cycle, which should generate an error message, but not this one. But (wky17 has told me) even if one replaces the wires by registers, the same error message is produced. If one “beefs up the constraint solver”, a construct similar to wky17’s can be used to give a maximum width constraint1—something that I would rather not have. Currently, there is a kind of monotonicity in width inference: if some width is ok (i.e. avoids data loss), then any larger width is allowed as well, only that it uses more silicon. However, that would no longer hold if one can formulate maximum width constraints. Footnotes
|
This is an example of GCD:
We can see that there is a dependency from y to x:
Then why there is no error in GCD program? Does it means that CIRCT just ignore cycles and solve with explicit widths? |
We tried a bit and found out that in circuit However, a similar trick does not work in the initial example In trying, we also found some other behaviour contrary to specification: “width( |
This is working because there is a constraint from a known width, specifically I do want to state that both of you are applying far more rigor to this problem than has been applied previously. Thanks for digging into this!
Yes, this sounds like a bug. If you do add the constraint, does the circuit optimize itself to nothing, i.e., just
I'm not sure what to do with this. I would be happy with a saner user error. E.g., "
Is this a CIRCT version issue or a bug where this was missed when making this spec change in CIRCT? This was only changed recently such that
@darthscsi: Do you have any thoughts on how you had been thinking about a new algorithm for width inference? |
When I write a FIRRTL program like this:
widths constraints for implicit widths in the circuit should be:
and the solution can be (2,1) or (1,2), which is confused to find out which solution should be assigned to a1,a2.
In fact for this case, firtool gives an error like this:
The error means this program is illegal For FIRRTL or there is some problem in CIRCT compiler? What should be the solution and the widths?
The text was updated successfully, but these errors were encountered: