llvm / llvm/circt

[FIRRTL] Tighten Layerblock Checking of Writes Outside the Layerblock

Open
#7,485 0 comments 0 reactions 0 assignees View on GitHub
bug FIRRTL
Dominant language
C++
Stars
2.2k
Forks
524
Avg merge
3d 2h
Merged PRs (30d)
46

Description

A `firrtl.layerblock` is supposed to contain no operations which write outside the `layerblock`. Improve the verifier for the `layerblock` to be able to check this.

The problem is specifically with a `connect` involving non-passively typed aggregate operands. When the operands have this type, one of three things could be happening:

1. The source could be driving the destination.
2. The destination could be driving the source.
3. The source and destination could be driving each other, i.e., both (1) and (2) for some ground types.

I made a decision in https://github.com/llvm/circt/pull/7462 to _not_ verify this as it is tricky and we didn't have infrastructure in place to do it. Additionally, the aggressive conversion of `connect` to `matchingconnect` (which requires passive types) almost immediately makes this verifier unnecessary. Additionally, anything parsed from FIRRTL text will use the `emitConnect` utility which intentionally doesn't create constructions like below. This can likely only come up for manually crafted MLIR or by passes that choose to create connects like this. That said, not checking this is far from ideal.

An example of problematic connects that should be rejected are:

``` mlir
firrtl.module @ConnectTests() {
%a = firrtl.wire : !firrtl.uint<1>
%b = firrtl.wire : !firrtl.bundle>
%c = firrtl.wire : !firrtl.bundle>
%d = firrtl.wire : !firrtl.bundle
>>
%e = firrtl.wire : !firrtl.bundle
>>
firrtl.layerblock @A {
%_a = firrtl.wire : !firrtl.uint<1>
%_b = firrtl.wire : !firrtl.bundle>
%_c = firrtl.wire : !firrtl.bundle
>
%_d = firrtl.wire : !firrtl.bundle
>>
%_e = firrtl.wire : !firrtl.bundle
>>

firrtl.connect %a, %_a : !firrtl.uint<1>
firrtl.connect %b, %_b : !firrtl.bundle>
firrtl.connect %_c, %c : !firrtl.bundle
>
firrtl.connect %_d, %d : !firrtl.bundle
>>
firrtl.connect %e, %_e : !firrtl.bundle
>>
}
}
```

Note that all of these examples are cases of (1) or (2). No case of (3) is shown. That would occur if you have a type which contains a bundle which has opposite direction leaves.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start by locating the firrtl.layerblock verifier and the connect handling it uses for non-passively typed aggregate operands. Review the listed examples as verifier cases, then make sure connects that write outside the layerblock are rejected, including the possible bidirectional case, while valid connects remain accepted.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
compilers
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.