[FIRRTL] Improve Domain Errors v2
Nobody has claimed this yet.
- Dominant language
- C++
- Stars
- 2.2k
- Forks
- 524
- Avg merge
- 3d 2h
- Merged PRs (30d)
- 46
Description
The current domain inference errors can be further improved beyond what was recorded in #10041 and implemented in #10337. Specifically, while the current version provides information about _where_ a domain came from, it doesn't provide information on _why_ two domains unified. Consider the following circuit where `Bar` is an incorrectly specified clock domain synchronizer:
``` firrtl
FIRRTL version 7.0.0
circuit Foo:
domain ClockDomain:
module Bar:
input a: UInt<1>
output b: UInt<1>
connect b, a
public module Foo:
input A: Domain of ClockDomain
input B: Domain of ClockDomain
input a: UInt<1> domains [A]
output b: UInt<1> domains [B]
inst bar of Bar
connect bar.a, a
connect b, bar.b
```
When compiled, this produces:
``` console
# firtool -domain-mode=infer-all circt/domain-error-improve.fir
circt/domain-error-improve.fir:21:5: error: illegal domain crossing in operation between operands b and bar.b
connect b, bar.b
^
circt/domain-error-improve.fir:21:5: note: see current operation: "firrtl.matchingconnect"(%arg3, %0#2) : (!firrtl.uint<1>, !firrtl.uint<1>) -> ()
circt/domain-error-improve.fir:16:12: note: b has domains [B : ClockDomain]
output b: UInt<1> domains [B]
^
circt/domain-error-improve.fir:19:5: note: bar.b has domains [A : ClockDomain]
inst bar of Bar
^
circt/domain-error-improve.fir:14:11: note: input module port B declared here
input B: Domain of ClockDomain
^
circt/domain-error-improve.fir:13:11: note: input module port A declared here
input A: Domain of ClockDomain
^
```
The information provided records the following pieces of information:
1. The top-level connect `connect b, bar.b` has an illegal domain crossing and it correctly identifies what are the two domains, `A` and `B`.
2. It identifies with good names all of the problematic connections and where they live.
However, it critically doesn't provide any information on _why_ the domain inference algorithm is trying to unify them. Or: the simple fix is that the failing connection needs to have a domain cast added. However, this is not the correct fix here. Instead, the fix is to add a domain cast at the connect inside `Bar`.
This is tricky and I don't have a good line on a solution. However, I have a couple of ideas:
1. After a unification error is found, it's almost like you want to have a way to re-run domain inference, but indicate that these two domains should never be merged. The hard part about this, is you don't know what should be included in the set of operations that you don't want to merge.
2. The algorithm working bottom-up is the issue and this should work more from known points in the design. We know when the algorithm starts that `bar.a` and `bar.b` have to be on different domains because we know about the domains of `Foo`. However, this information is not used in the inference of `Bar`. If we continue to allow for the `-domain-mode=infer-all` mode of operation, then it's not a strict "top-down" is better than "bottom-up" and it starts to look more like the algorithm works in multiple directions at once based on the known domain information it has.
Both of these ideas view the unification slightly differently where it's more like you have two expanding bubbles of connectivity and you're looking for the points at which they intersect. Any of those intersections are then wrong and most likely to be the places where uses need to make modifications. Granted, this may be a function of the examples I'm seeing and not a truly generally property. I.e., this seems to optimize for the most distant unification points being the identified problems, when theoretically, any connection point could be the issue.
Regardless, the current information is proving insufficient for me (a developer) to use this and it will certainly be too much for a user. My current loop here is to rely on `circt-reduce` with a number of optimizations that inline or rename things turned off so that I can get a smaller circuit that still has all the same names, but also has the same domain crossing error.
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with the domain inference behavior exercised by firtool using the FIRRTL reproducer in the issue, and compare the reported unification path with the intended cast location inside Bar. Use circt-reduce as described to produce smaller circuits while preserving names and the domain-crossing error. Done means domain errors explain why domains were unified and guide the user toward the correct cast location.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- compilers
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100