[Arc] Partially enable reset/enable detection (#6506)

Enable the `InferStateProperties` pass in the arcilator pipeline and
make its enable and reset signal detection individually controllable.
The enable portion is already supported by the rest of the arcilator
pipeline and can produce 20%-35% speedup on the cores in arc-tests.
Turn on enable detection by default.

The reset portion is not fully supported yet and causes the simulation
to misbehave. It is disabled by default.

As a minor refactoring this removes the `constructor` field from the
pass definition, such that the constructor and plumbing for options gets
generated automatically. As a side effect, the constructor is now called
`arc::createInferStateProperties` instead of the previous
`arc::createInferStatePropertiesPass`. (Thanks @uenoku for the pointer.)

Shoutout to @maerhart and @TaoBi22 for this fantastic pass!

include/circt/Dialect/Arc/ArcPasses.h

@@ -33,7 +33,6 @@ std::unique_ptr<mlir::Pass> createDedupPass();
 std::unique_ptr<mlir::Pass> createGroupResetsAndEnablesPass();
 std::unique_ptr<mlir::Pass>
 createInferMemoriesPass(const InferMemoriesOptions &options = {});
-std::unique_ptr<mlir::Pass> createInferStatePropertiesPass();
 std::unique_ptr<mlir::Pass> createInlineArcsPass();
 std::unique_ptr<mlir::Pass> createInlineModulesPass();
 std::unique_ptr<mlir::Pass> createIsolateClocksPass();

include/circt/Dialect/Arc/ArcPasses.td

@@ -114,8 +114,21 @@ def InlineModules : Pass<"arc-inline-modules", "mlir::ModuleOp"> {
 def InferStateProperties : Pass<"arc-infer-state-properties",
                                 "mlir::ModuleOp"> {
   let summary = "Add resets and enables explicitly to the state operations";
-  let constructor = "circt::arc::createInferStatePropertiesPass()";
   let dependentDialects = ["circt::hw::HWDialect", "circt::comb::CombDialect"];
+  let options = [
+    Option<"detectEnables", "enables", "bool", "true", "Infer enable signals">,
+    Option<"detectResets", "resets", "bool", "true", "Infer reset signals">,
+  ];
+  let statistics = [
+    Statistic<"addedEnables", "added-enables",
+      "Enables added explicitly to a StateOp">,
+    Statistic<"addedResets", "added-resets",
+      "Resets added explicitly to a StateOp">,
+    Statistic<"missedEnables", "missed-enables",
+      "Detected enables that could not be added explicitly to a StateOp">,
+    Statistic<"missedResets", "missed-resets",
+      "Detected resets that could not be added explicitly to a StateOp">,
+  ];
 }
 
 def IsolateClocks : Pass<"arc-isolate-clocks", "mlir::ModuleOp"> {

lib/Dialect/Arc/Transforms/InferStateProperties.cpp

@@ -380,25 +380,12 @@ EnableInfo computeEnableInfoFromPattern(OpOperand &output, StateOp stateOp) {
 
 namespace {
 struct InferStatePropertiesPass
-    : public arc::impl::InferStatePropertiesBase<InferStatePropertiesPass> {
-  InferStatePropertiesPass() = default;
-  InferStatePropertiesPass(const InferStatePropertiesPass &pass)
-      : InferStatePropertiesPass() {}
+    : public impl::InferStatePropertiesBase<InferStatePropertiesPass> {
+  using InferStatePropertiesBase::InferStatePropertiesBase;
 
   void runOnOperation() override;
   void runOnStateOp(arc::StateOp stateOp, arc::DefineOp arc,
                     DenseMap<arc::DefineOp, unsigned> &resetConditionMap);
-
-  Statistic addedEnables{this, "added-enables",
-                         "Enables added explicitly to a StateOp"};
-  Statistic addedResets{this, "added-resets",
-                        "Resets added explicitly to a StateOp"};
-  Statistic missedEnables{
-      this, "missed-enables",
-      "Detected enables that could not be added explicitly to a StateOp"};
-  Statistic missedResets{
-      this, "missed-resets",
-      "Detected resets that could not be added explicitly to a StateOp"};
 };
 } // namespace
 
@@ -422,56 +409,55 @@ void InferStatePropertiesPass::runOnStateOp(
     return;
 
   auto outputOp = cast<arc::OutputOp>(arc.getBodyBlock().getTerminator());
-  const unsigned visitedNoChange = -1;
-
-  // Check for reset patterns, we only have to do this once per arc::DefineOp
-  // and store the result for later arc::StateOps referring to the same arc.
-  if (!resetConditionMap.count(arc)) {
-    SmallVector<ResetInfo> resetInfos;
-    int numResets = 0;
-    ;
-    for (auto &output : outputOp->getOpOperands()) {
-      auto resetInfo = computeResetInfoFromPattern(output);
-      resetInfos.push_back(resetInfo);
-      if (resetInfo)
-        ++numResets;
-    }
-
-    // Rewrite the arc::DefineOp if valid
-    auto result = applyResetTransformation(arc, resetInfos);
-    if ((succeeded(result) && resetInfos[0]))
-      resetConditionMap[arc] = resetInfos[0].condition.getArgNumber();
-    else
-      resetConditionMap[arc] = visitedNoChange;
+  static constexpr unsigned visitedNoChange = -1;
+
+  if (detectResets) {
+    // Check for reset patterns, we only have to do this once per arc::DefineOp
+    // and store the result for later arc::StateOps referring to the same arc.
+    if (!resetConditionMap.count(arc)) {
+      SmallVector<ResetInfo> resetInfos;
+      int numResets = 0;
+      for (auto &output : outputOp->getOpOperands()) {
+        auto resetInfo = computeResetInfoFromPattern(output);
+        resetInfos.push_back(resetInfo);
+        if (resetInfo)
+          ++numResets;
+      }
 
-    if (failed(result))
-      missedResets += numResets;
-  }
+      // Rewrite the arc::DefineOp if valid
+      auto result = applyResetTransformation(arc, resetInfos);
+      if ((succeeded(result) && resetInfos[0]))
+        resetConditionMap[arc] = resetInfos[0].condition.getArgNumber();
+      else
+        resetConditionMap[arc] = visitedNoChange;
 
-  // Apply resets to the state operation.
-  if (resetConditionMap.count(arc) &&
-      resetConditionMap[arc] != visitedNoChange) {
-    setResetOperandOfStateOp(stateOp, resetConditionMap[arc]);
-    ++addedResets;
-  }
+      if (failed(result))
+        missedResets += numResets;
+    }
 
-  // Check for enable patterns.
-  SmallVector<EnableInfo> enableInfos;
-  int numEnables = 0;
-  for (OpOperand &output : outputOp->getOpOperands()) {
-    auto enableInfo = computeEnableInfoFromPattern(output, stateOp);
-    enableInfos.push_back(enableInfo);
-    if (enableInfo)
-      ++numEnables;
+    // Apply resets to the state operation.
+    if (resetConditionMap.count(arc) &&
+        resetConditionMap[arc] != visitedNoChange) {
+      setResetOperandOfStateOp(stateOp, resetConditionMap[arc]);
+      ++addedResets;
+    }
   }
 
-  // Apply enable patterns.
-  if (!failed(applyEnableTransformation(arc, stateOp, enableInfos)))
-    ++addedEnables;
-  else
-    missedEnables += numEnables;
-}
+  if (detectEnables) {
+    // Check for enable patterns.
+    SmallVector<EnableInfo> enableInfos;
+    int numEnables = 0;
+    for (OpOperand &output : outputOp->getOpOperands()) {
+      auto enableInfo = computeEnableInfoFromPattern(output, stateOp);
+      enableInfos.push_back(enableInfo);
+      if (enableInfo)
+        ++numEnables;
+    }
 
-std::unique_ptr<Pass> arc::createInferStatePropertiesPass() {
-  return std::make_unique<InferStatePropertiesPass>();
+    // Apply enable patterns.
+    if (!failed(applyEnableTransformation(arc, stateOp, enableInfos)))
+      ++addedEnables;
+    else
+      missedEnables += numEnables;
+  }
 }

tools/arcilator/arcilator.cpp

@@ -108,6 +108,16 @@ static cl::opt<bool> shouldInline("inline", cl::desc("Inline arcs"),
 static cl::opt<bool> shouldDedup("dedup", cl::desc("Deduplicate arcs"),
                                  cl::init(true), cl::cat(mainCategory));
 
+static cl::opt<bool> shouldDetectEnables(
+    "detect-enables",
+    cl::desc("Infer enable conditions for states to avoid computation"),
+    cl::init(true), cl::cat(mainCategory));
+
+static cl::opt<bool> shouldDetectResets(
+    "detect-resets",
+    cl::desc("Infer reset conditions for states to avoid computation"),
+    cl::init(false), cl::cat(mainCategory));
+
 static cl::opt<bool>
     shouldMakeLUTs("lookup-tables",
                    cl::desc("Optimize arcs into lookup tables"), cl::init(true),
@@ -236,19 +246,19 @@ static void populatePipeline(PassManager &pm) {
   pm.addPass(arc::createSplitLoopsPass());
   if (shouldDedup)
     pm.addPass(arc::createDedupPass());
+  {
+    arc::InferStatePropertiesOptions opts;
+    opts.detectEnables = shouldDetectEnables;
+    opts.detectResets = shouldDetectResets;
+    pm.addPass(arc::createInferStateProperties(opts));
+  }
   pm.addPass(createCSEPass());
   pm.addPass(arc::createArcCanonicalizerPass());
   if (shouldMakeLUTs)
     pm.addPass(arc::createMakeTablesPass());
   pm.addPass(createCSEPass());
   pm.addPass(arc::createArcCanonicalizerPass());
 
-  // TODO: the following is commented out because the backend does not support
-  // StateOp resets yet.
-  // pm.addPass(arc::createInferStatePropertiesPass());
-  // InferStateProperties does not remove all ops it bypasses and inserts a lot
-  // of constant ops that should be uniqued
-  // pm.addPass(createSimpleCanonicalizerPass());
   // Now some arguments may be unused because reset conditions are not passed as
   // inputs anymore pm.addPass(arc::createRemoveUnusedArcArgumentsPass());
   // Because we replace a lot of StateOp inputs with constants in the enable