[FIRRTL] Lint XMRs in the "design" #8668

seldridge · 2025-07-08T15:20:49Z

Add an option to FIRRTL's linter (and expose it up to firtool) that will
lint (error) if any XMRs exist in the "design". This is intended to catch
problems where downstream consumers of CIRCT-generated Verilog (e.g.,
Physical Design teams) get unexpected unsynthesizable XMRs in the design.
This is almost never what you want.

Unfortunately, this is highly error prone. It's difficult to tell what
is/is not the "design". This relies on the InstanceInfo analysis and
intentionally uses its notion of "design" and not "effective design". In
other words, this will only trigger this kind of linting if there is an
explicit MarkDUTAnnotation. It will not assume that the main module has
an implicit MarkDUTAnnotation. This is intended to avoid false
positives.

In order to make this work, there is a lot of prequisite work batched in
this PR. A new pass, which only exists to run the InstanceInfo analysis
(and thereby cache it) is created. This allows for use of the
getCachedParentAnalysis function and guaranteeing that it will succeed.
(This is a pattern borrowed from IREE.) Options are added for the
singular, existing lint check as well.

This PR is best reviewed as individual commits. I am intending to land
this as individual commits as well.

uenoku

If it was necessary to add a new pass to cache InstanceInfo can we just make linter pass circuit op pass? For this specific use case it's safe since Lint pass doesn't mutate operations and circuit passes are executed before and after Lint pass but it means you cannot put Lint pass to arbitrary place

seldridge · 2025-07-08T19:12:15Z

@uenoku: That's the alternative way to proceed here.

I was toying with this approach as it seemed beneficial to keep the FModuleOp parallelism exposed to MLIR infrastructure as opposed to manually managing it. However, there appear to be no actual users of getCachedParentAnalysis other than that one user in IREE.

lib/Dialect/FIRRTL/Transforms/Lint.cpp

lib/Firtool/Firtool.cpp

uenoku · 2025-07-08T23:16:02Z

lib/Firtool/Firtool.cpp

+  llvm::cl::opt<bool> lintStaticAsserts{
+      "lint-static-asserts", llvm::cl::desc("Lint static assertions"),
+      llvm::cl::init(true)};
+  llvm::cl::opt<bool> lintXmrsInDesign{
+      "lint-xmrs-in-design", llvm::cl::desc("Lint XMRs in the design"),
+      llvm::cl::init(true)};


Do we need options to disable them? 🤔 I feel it's beneficial to run always.

We get that for free with -lint-xmrs-in-design=0 / -lint-xmrs-in-design=false. This is slightly not great as users may not realize that they can disable these like that. WDYT?

I think having options are good for me as long as there are use cases.

uenoku

LGTM

Convert the FIRRTL linting pass to be a `CircuitOp` pass with internally handled parallelism. This is done in preparation to add XMR linting which requires an `InstanceInfo` analysis which needs to be on the `CircuitOp`. There are two ways of doing this: 1. Make the pass operate on the `CircuitOp` as this patch does. 2. Use a cached parent analysis which requires a dedicated pass to compute the `InstanceInfo` analysis and then careful pass pipeline structuring. (2): is used in one place in IREE and nowhere else. Additionally, (1) was suggested by multiple people in "Hallway Usabilty Testing"-type discussions. (Many people thought (2) was odd.) Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Make the linting of static assertions an option to the linter. While this appears unnecessary now, I am intending to add additional lint checks to the linter and I want to be able to turn each of them off if they become problematic. Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Expose an option to turn off/on the linting of trivially false assertions. This option defaults to "on" because this was the behavior originally. Also, I am intending to follow this going forward where lint checks are always on and have to be turned off. Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Extend the FIRRTL linter to check that there are no XMRs that will be located in the "design". This intentionally only checks for problems in the "design" and NOT the "effective design". This errs on the side of having more false negatives without introducing the possibility of false positives for separate compilation flows. Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Add an option to FIRRTL's linter that allows for disabling/enabling linting of XMRs that exist in the "design". Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Add an option to the `firtool` command line that allows for disabling/enabling the linting of XMRs that exist in the "design". While there is now a general policy of exposing each lint option in firtool, I am particularly nervous about this option as it has a high likelihood of causing problems (false positives). Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

@uenoku

Refactor FIRRTL's Lint pass to use the ODS-generated constructor as opposed to hand-rolling one. This avoids weirdness where the command-line options have different defaults from what they are specified as in ODS and avoids the need for a "Config" struct inside the linter's namespace (as the ODS-generated one can be used). h/t @uenoku for the suggestion. Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

seldridge requested a review from darthscsi as a code owner July 8, 2025 15:20

seldridge requested review from rwy7 and prithayan July 8, 2025 15:21

seldridge force-pushed the dev/seldridge/firrtl-lint-design-xmrs branch from 3ec3df5 to 6e96938 Compare July 8, 2025 15:30

seldridge requested a review from uenoku July 8, 2025 15:33