[firtool] Only run verifiers at required points in the pipeline

include/circt/Firtool/Firtool.h

@@ -34,6 +34,18 @@ class FirtoolOptions {
   FirtoolOptions();
 
   // Helper Types
+
+  /// Verification mode for the firtool pipeline.
+  enum class VerificationMode {
+    /// Run verification after every pass (using built-in pass manager
+    /// verification).
+    All,
+    /// Run verification only when required.
+    Default,
+    /// Do not run verification.
+    None
+  };
+
   enum BuildMode { BuildModeDefault, BuildModeDebug, BuildModeRelease };
   enum class RandomKind { None, Mem, Reg, All };
 
@@ -179,6 +191,20 @@ class FirtoolOptions {
 
   DomainMode getDomainMode() const { return domainMode; }
 
+  VerificationMode getVerificationMode() const { return verificationMode; }
+
+  bool isVerificationModeAll() const {
+    return verificationMode == VerificationMode::All;
+  }
+
+  bool isVerificationModeDefault() const {
+    return verificationMode == VerificationMode::Default;
+  }
+
+  bool isVerificationModeNone() const {
+    return verificationMode == VerificationMode::None;
+  }
+
   // Setters, used by the CAPI
   FirtoolOptions &setOutputFilename(StringRef name) {
     outputFilename = name;
@@ -438,6 +464,11 @@ class FirtoolOptions {
     return *this;
   }
 
+  FirtoolOptions &setVerificationMode(VerificationMode value) {
+    verificationMode = value;
+    return *this;
+  }
+
 private:
   std::string outputFilename;
 
@@ -494,6 +525,7 @@ class FirtoolOptions {
   bool emitAllBindFiles;
   bool inlineInputOnlyModules;
   DomainMode domainMode;
+  VerificationMode verificationMode;
 };
 
 void registerFirtoolCLOptions();

include/circt/Support/Passes.h

@@ -59,6 +59,13 @@ class VerbosePassInstrumentation : public mlir::PassInstrumentation {
 /// Create a simple canonicalizer pass.
 std::unique_ptr<Pass> createSimpleCanonicalizerPass();
 
+/// Create a verifier pass that always runs verification. Unlike the built-in
+/// pass manager verification (enabled via pm.enableVerifier()), this pass
+/// always runs verification regardless of whether the previous pass preserved
+/// all analyses. This is useful for strategic verification points in the
+/// pipeline when using verify=default mode.
+std::unique_ptr<Pass> createVerifierPass();
+
 } // namespace circt
 
 #endif // CIRCT_SUPPORT_PASSES_H

lib/Dialect/FIRRTL/Import/FIRParser.cpp

@@ -6532,12 +6532,6 @@ circt::firrtl::importFIRFile(SourceMgr &sourceMgr, MLIRContext *context,
           .parseCircuit(annotationsBufs, ts))
     return nullptr;
 
-  // Make sure the parse module has no other structural problems detected by
-  // the verifier.
-  auto circuitVerificationTimer = ts.nest("Verify circuit");
-  if (failed(verify(*module)))
-    return {};
-
   return module;
 }
 

lib/Firtool/Firtool.cpp

@@ -26,6 +26,14 @@ using namespace circt;
 
 LogicalResult firtool::populatePreprocessTransforms(mlir::PassManager &pm,
                                                     const FirtoolOptions &opt) {
+  // Configure built-in pass manager verification based on mode.
+  if (!opt.isVerificationModeAll())
+    pm.enableVerifier(false);
+
+  // Run initial verification for "default" mode.
+  if (opt.isVerificationModeDefault())
+    pm.addPass(createVerifierPass());
+
   pm.nest<firrtl::CircuitOp>().addPass(
       firrtl::createCheckRecursiveInstantiation());
   pm.nest<firrtl::CircuitOp>().addPass(firrtl::createCheckLayers());
@@ -39,6 +47,10 @@ LogicalResult firtool::populatePreprocessTransforms(mlir::PassManager &pm,
        /*ignoreAnnotationUnknown=*/opt.shouldDisableUnknownAnnotations(),
        /*noRefTypePorts=*/opt.shouldLowerNoRefTypePortAnnotations()}));
 
+  // Run verifiers after annotation handlers run.
+  if (opt.isVerificationModeDefault())
+    pm.addPass(createVerifierPass());
+
   if (opt.shouldEnableDebugInfo())
     pm.nest<firrtl::CircuitOp>().addNestedPass<firrtl::FModuleOp>(
         firrtl::createMaterializeDebugInfo());
@@ -100,6 +112,11 @@ LogicalResult firtool::populateCHIRRTLToLowFIRRTL(mlir::PassManager &pm,
 
   pm.nest<firrtl::CircuitOp>().addPass(firrtl::createInferResets());
 
+  // Run verifiers after InferResets, as the full asynchronous reset transform
+  // is relying on verification.
+  if (opt.isVerificationModeDefault())
+    pm.addPass(createVerifierPass());
+
   pm.nest<firrtl::CircuitOp>().addPass(firrtl::createDropConst());
 
   if (opt.shouldDedup()) {
@@ -288,7 +305,8 @@ LogicalResult firtool::populateLowFIRRTLToHW(mlir::PassManager &pm,
   pm.nest<firrtl::CircuitOp>().addPass(firrtl::createLowerDPI());
   pm.nest<firrtl::CircuitOp>().addPass(firrtl::createLowerDomains());
   pm.nest<firrtl::CircuitOp>().addPass(firrtl::createLowerClasses());
-  pm.nest<firrtl::CircuitOp>().addPass(om::createVerifyObjectFieldsPass());
+  if (opt.isVerificationModeAll())
+    pm.nest<firrtl::CircuitOp>().addPass(om::createVerifyObjectFieldsPass());
 
   // Check for static asserts.
   pm.nest<firrtl::CircuitOp>().addPass(circt::firrtl::createLint(
@@ -304,11 +322,12 @@ LogicalResult firtool::populateLowFIRRTLToHW(mlir::PassManager &pm,
     modulePM.addPass(createSimpleCanonicalizerPass());
   }
 
-  // Check inner symbols and inner refs.
-  pm.addPass(hw::createVerifyInnerRefNamespace());
-
-  // Check OM object fields.
-  pm.addPass(om::createVerifyObjectFieldsPass());
+  if (opt.isVerificationModeAll()) {
+    // Check inner symbols and inner refs.
+    pm.addPass(hw::createVerifyInnerRefNamespace());
+    // Check OM object fields.
+    pm.addPass(om::createVerifyObjectFieldsPass());
+  }
 
   // Run the verif op verification pass
   pm.addNestedPass<hw::HWModuleOp>(verif::createVerifyClockedAssertLikePass());
@@ -362,11 +381,12 @@ LogicalResult firtool::populateHWToSV(mlir::PassManager &pm,
         /*mergeAlwaysBlocks=*/!opt.shouldEmitSeparateAlwaysBlocks()));
   }
 
-  // Check inner symbols and inner refs.
-  pm.addPass(hw::createVerifyInnerRefNamespace());
-
   // Check OM object fields.
-  pm.addPass(om::createVerifyObjectFieldsPass());
+  if (opt.isVerificationModeAll()) {
+    // Check inner symbols and inner refs.
+    pm.addPass(hw::createVerifyInnerRefNamespace());
+    pm.addPass(om::createVerifyObjectFieldsPass());
+  }
 
   return success();
 }
@@ -375,7 +395,6 @@ namespace detail {
 LogicalResult
 populatePrepareForExportVerilog(mlir::PassManager &pm,
                                 const firtool::FirtoolOptions &opt) {
-
   // Run the verif op verification pass
   pm.addNestedPass<hw::HWModuleOp>(verif::createVerifyClockedAssertLikePass());
 
@@ -401,11 +420,12 @@ populatePrepareForExportVerilog(mlir::PassManager &pm,
   if (opt.shouldExportModuleHierarchy())
     pm.addPass(sv::createHWExportModuleHierarchyPass());
 
-  // Check inner symbols and inner refs.
-  pm.addPass(hw::createVerifyInnerRefNamespace());
-
-  // Check OM object fields.
-  pm.addPass(om::createVerifyObjectFieldsPass());
+  if (opt.isVerificationModeAll()) {
+    // Check inner symbols and inner refs.
+    pm.addPass(hw::createVerifyInnerRefNamespace());
+    // Check OM object fields.
+    pm.addPass(om::createVerifyObjectFieldsPass());
+  }
 
   return success();
 }
@@ -800,6 +820,23 @@ struct FirtoolCmdOptions {
   llvm::cl::opt<bool> lintXmrsInDesign{
       "lint-xmrs-in-design", llvm::cl::desc("Lint XMRs in the design"),
       llvm::cl::init(false)};
+
+  //===----------------------------------------------------------------------===
+  // Verification options
+  //===----------------------------------------------------------------------===
+
+  llvm::cl::opt<firtool::FirtoolOptions::VerificationMode> verificationMode{
+      "verify", llvm::cl::desc("Specify when to run verification"),
+      llvm::cl::values(
+          clEnumValN(firtool::FirtoolOptions::VerificationMode::All, "all",
+                     "Run the verifier after each transformation pass"),
+          clEnumValN(firtool::FirtoolOptions::VerificationMode::Default,
+                     "default",
+                     "Run the verifier at required points in the pipeline"),
+          clEnumValN(firtool::FirtoolOptions::VerificationMode::None, "none",
+                     "Do not run the verifier")),
+      llvm::cl::init(firtool::FirtoolOptions::VerificationMode::Default),
+      llvm::cl::Hidden};
 };
 } // namespace
 
@@ -840,7 +877,8 @@ circt::firtool::FirtoolOptions::FirtoolOptions()
       symbolicValueLowering(verif::SymbolicValueLowering::ExtModule),
       disableWireElimination(false), lintStaticAsserts(true),
       lintXmrsInDesign(true), emitAllBindFiles(false),
-      inlineInputOnlyModules(false), domainMode(DomainMode::Disable) {
+      inlineInputOnlyModules(false), domainMode(DomainMode::Disable),
+      verificationMode(VerificationMode::Default) {
   if (!clOptions.isConstructed())
     return;
   outputFilename = clOptions->outputFilename;
@@ -895,4 +933,5 @@ circt::firtool::FirtoolOptions::FirtoolOptions()
   emitAllBindFiles = clOptions->emitAllBindFiles;
   inlineInputOnlyModules = clOptions->inlineInputOnlyModules;
   domainMode = clOptions->domainMode;
+  verificationMode = clOptions->verificationMode;
 }

lib/Support/Passes.cpp

@@ -7,10 +7,34 @@
 //===----------------------------------------------------------------------===//
 
 #include "circt/Support/Passes.h"
+#include "mlir/IR/Verifier.h"
+#include "mlir/Pass/Pass.h"
 #include "mlir/Transforms/GreedyPatternRewriteDriver.h"
 #include "mlir/Transforms/Passes.h"
 
 using namespace circt;
+using namespace mlir;
+
+namespace circt {
+/// A pass that runs verification on the operation. This pass always runs
+/// verification, unlike the built-in pass manager verification which can be
+/// skipped if the previous pass preserved all analyses.
+struct VerifierPass : public PassWrapper<VerifierPass, OperationPass<>> {
+  MLIR_DEFINE_EXPLICIT_INTERNAL_INLINE_TYPE_ID(VerifierPass)
+
+  StringRef getArgument() const override { return "verify-ir"; }
+  StringRef getDescription() const override {
+    return "Verify the IR at this point in the pipeline";
+  }
+
+  void runOnOperation() override {
+    if (failed(mlir::verify(getOperation())))
+      return signalPassFailure();
+    // Mark all analyses as preserved since we didn't modify anything
+    markAllAnalysesPreserved();
+  }
+};
+} // namespace circt
 
 std::unique_ptr<Pass> circt::createSimpleCanonicalizerPass() {
   mlir::GreedyRewriteConfig config;
@@ -19,3 +43,7 @@ std::unique_ptr<Pass> circt::createSimpleCanonicalizerPass() {
       mlir::GreedySimplifyRegionLevel::Disabled);
   return mlir::createCanonicalizerPass(config);
 }
+
+std::unique_ptr<Pass> circt::createVerifierPass() {
+  return std::make_unique<VerifierPass>();
+}

test/Dialect/HW/verify-irn.mlir

@@ -1,7 +1,5 @@
 // Check explicit verification pass.
 // RUN: circt-opt -hw-verify-irn -verify-diagnostics -split-input-file %s
-// Check verification occurs in firtool pipeline.
-// RUN: firtool -verify-diagnostics -split-input-file %s
 
 // #3526
 hw.module @B() {}

test/firtool/commandline.mlir

@@ -1,4 +1,5 @@
 // RUN: firtool --help | FileCheck %s --implicit-check-not='{{[Oo]}}ptions:'
+// RUN: firtool --help-hidden | FileCheck %s --check-prefix=HIDDEN
 
 // CHECK: OVERVIEW: MLIR-based FIRRTL compiler
 // CHECK: General {{[Oo]}}ptions
@@ -7,3 +8,9 @@
 // CHECK-DAG: -j{{.*}}Alias for --num-threads
 // CHECK-DAG: --lowering-options=
 // CHECK-DAG: --num-threads=<N>{{.*}}Number of threads to use for parallel compilation
+
+// HIDDEN: --verify=<value>
+// HIDDEN-SAME: Specify when to run verification
+// HIDDEN-DAG: =all
+// HIDDEN-DAG: =default
+// HIDDEN-DAG: =none

test/firtool/verify-modes.fir

@@ -0,0 +1,20 @@
+; Test that the --verify option works with all modes
+; This test contains invalid FIRRTL (layerblock connecting to outside destination)
+;
+; With verify=all or verify=default, the verifier should catch the layerblock error
+; RUN: not firtool %s --parse-only --verify=all 2>&1 | FileCheck %s --check-prefix=ERROR
+; RUN: not firtool %s --parse-only --verify=default 2>&1 | FileCheck %s --check-prefix=ERROR
+;
+; With verify=none, the verifier error is not caught.
+; RUN: firtool %s --parse-only --verify=none | FileCheck %s --check-prefix=SUCCESS
+
+FIRRTL version 4.0.0
+; SUCCESS: "VerifyTest"
+circuit VerifyTest :
+  layer A, bind:
+  public module VerifyTest :
+    input a : UInt<8>
+    output b : UInt<8>
+    layerblock A:
+      ; ERROR: op connects to a destination which is defined outside its enclosing layer block
+      connect b, a

tools/circt-test/circt-test.cpp

@@ -1002,6 +1002,11 @@ static LogicalResult executeWithHandler(MLIRContext *context,
   llvm::sys::path::append(verilogPath, "design");
   firtoolOptions.setOutputFilename(verilogPath);
 
+  // Set verification mode to All if verifyPasses is enabled.
+  if (opts.verifyPasses)
+    firtoolOptions.setVerificationMode(
+        firtool::FirtoolOptions::VerificationMode::All);
+
   PassManager pm(context);
   pm.enableVerifier(opts.verifyPasses);
   if (failed(firtool::populateHWToSV(pm, firtoolOptions)))

tools/firtool/firtool.cpp

@@ -46,6 +46,7 @@
 #include "mlir/Dialect/Func/IR/FuncOps.h"
 #include "mlir/IR/AsmState.h"
 #include "mlir/IR/BuiltinOps.h"
+#include "mlir/IR/Verifier.h"
 #include "mlir/Parser/Parser.h"
 #include "mlir/Pass/Pass.h"
 #include "mlir/Pass/PassInstrumentation.h"
@@ -201,11 +202,6 @@ static cl::opt<OutputFormatKind> outputFormat(
         clEnumValN(OutputDisabled, "disable-output", "Do not output anything")),
     cl::init(OutputVerilog), cl::cat(mainCategory));
 
-static cl::opt<bool>
-    verifyPasses("verify-each",
-                 cl::desc("Run the verifier after each transformation pass"),
-                 cl::init(true), cl::cat(mainCategory));
-
 static cl::list<std::string> inputAnnotationFilenames(
     "annotation-file", cl::desc("Optional input annotation file"),
     cl::CommaSeparated, cl::value_desc("filename"), cl::cat(mainCategory));
@@ -468,7 +464,6 @@ static LogicalResult processBuffer(
 
   // Apply any pass manager command line options.
   PassManager pm(&context);
-  pm.enableVerifier(verifyPasses);
   pm.enableTiming(ts);
   if (verbosePassExecutions)
     pm.addInstrumentation(