llvm / llvm/circt

[FIRParser] behavior with multiple circuits

Open
#2,978 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
C++
Stars
2.2k
Forks
524
Avg merge
3d 2h
Merged PRs (30d)
46

Description

# Summary

We currently accept multiple top-level circuit definitions when parsing FIRRTL and (IMO) produce unexpected IR when this is encountered. The presence of multiple circuits in one file does not appear to be valid FIRRTL, and as a result I think this should either be rejected, warned on, or supported in a more expected way (regardless of being in the spec). Personally I'd prefer to see it supported, but am unsure about the current behavior.

Presently we appear to ignore subsequent circuit declarations after the first and add any modules within to the first circuit (as if the later "circuit" lines were not present). An example helps demonstrate:

# Example

Example (`test.fir`):

```firrtl
circuit Top1:
module Top1:
skip

circuit Top2:
module Top2:
skip
```

## Current Behavior

Running `firtool --parse-only -format=fir test.fir` produces:

```mlir
module {
firrtl.circuit "Top1" {
firrtl.module @Top1() {
firrtl.skip
}
firrtl.module private @Top2() {
firrtl.skip
}
}
}
```

Notice there is no "Top2" circuit emitted, and its sole module (`@Top2`) is added into the first circuit.

## Possible alternate behavior

If supported, I'd expect something like this be produced:

```mlir
module {
firrtl.circuit "Top1" {
firrtl.module @Top1() {
firrtl.skip
}
}
firrtl.circuit "Top2" {
firrtl.module @Top2() {
firrtl.skip
}
}
}
```

That is, all circuits in the source are present and modules are within their respective circuits.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Reproduce the behavior with the test.fir example using `firtool --parse-only -format=fir test.fir` and compare the emitted MLIR with the issue's examples. Determine whether multiple top-level circuits should be supported, rejected, or warned on; done means the chosen behavior is implemented consistently rather than silently merging Top2 into Top1.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
compilers
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
30/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.