From 08665a1b1691d6b4b4d846aa40f6058d2c08879c Mon Sep 17 00:00:00 2001 From: Fabian Schuiki Date: Fri, 8 Dec 2023 15:21:34 -0800 Subject: [PATCH] [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 | 1 - include/circt/Dialect/Arc/ArcPasses.td | 15 ++- .../Arc/Transforms/InferStateProperties.cpp | 108 ++++++++---------- tools/arcilator/arcilator.cpp | 22 +++- 4 files changed, 77 insertions(+), 69 deletions(-) diff --git a/include/circt/Dialect/Arc/ArcPasses.h b/include/circt/Dialect/Arc/ArcPasses.h index 94782ff3b505..468d5635ee26 100644 --- a/include/circt/Dialect/Arc/ArcPasses.h +++ b/include/circt/Dialect/Arc/ArcPasses.h @@ -33,7 +33,6 @@ std::unique_ptr createDedupPass(); std::unique_ptr createGroupResetsAndEnablesPass(); std::unique_ptr createInferMemoriesPass(const InferMemoriesOptions &options = {}); -std::unique_ptr createInferStatePropertiesPass(); std::unique_ptr createInlineArcsPass(); std::unique_ptr createInlineModulesPass(); std::unique_ptr createIsolateClocksPass(); diff --git a/include/circt/Dialect/Arc/ArcPasses.td b/include/circt/Dialect/Arc/ArcPasses.td index ab0d3171a083..c14f9f3ee58b 100644 --- a/include/circt/Dialect/Arc/ArcPasses.td +++ b/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"> { diff --git a/lib/Dialect/Arc/Transforms/InferStateProperties.cpp b/lib/Dialect/Arc/Transforms/InferStateProperties.cpp index 8513ad440a9f..0a75a2ec9286 100644 --- a/lib/Dialect/Arc/Transforms/InferStateProperties.cpp +++ b/lib/Dialect/Arc/Transforms/InferStateProperties.cpp @@ -380,25 +380,12 @@ EnableInfo computeEnableInfoFromPattern(OpOperand &output, StateOp stateOp) { namespace { struct InferStatePropertiesPass - : public arc::impl::InferStatePropertiesBase { - InferStatePropertiesPass() = default; - InferStatePropertiesPass(const InferStatePropertiesPass &pass) - : InferStatePropertiesPass() {} + : public impl::InferStatePropertiesBase { + using InferStatePropertiesBase::InferStatePropertiesBase; void runOnOperation() override; void runOnStateOp(arc::StateOp stateOp, arc::DefineOp arc, DenseMap &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.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 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 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 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 enableInfos; + int numEnables = 0; + for (OpOperand &output : outputOp->getOpOperands()) { + auto enableInfo = computeEnableInfoFromPattern(output, stateOp); + enableInfos.push_back(enableInfo); + if (enableInfo) + ++numEnables; + } -std::unique_ptr arc::createInferStatePropertiesPass() { - return std::make_unique(); + // Apply enable patterns. + if (!failed(applyEnableTransformation(arc, stateOp, enableInfos))) + ++addedEnables; + else + missedEnables += numEnables; + } } diff --git a/tools/arcilator/arcilator.cpp b/tools/arcilator/arcilator.cpp index 560f89a49f54..c5b3ee3414e2 100644 --- a/tools/arcilator/arcilator.cpp +++ b/tools/arcilator/arcilator.cpp @@ -108,6 +108,16 @@ static cl::opt shouldInline("inline", cl::desc("Inline arcs"), static cl::opt shouldDedup("dedup", cl::desc("Deduplicate arcs"), cl::init(true), cl::cat(mainCategory)); +static cl::opt shouldDetectEnables( + "detect-enables", + cl::desc("Infer enable conditions for states to avoid computation"), + cl::init(true), cl::cat(mainCategory)); + +static cl::opt shouldDetectResets( + "detect-resets", + cl::desc("Infer reset conditions for states to avoid computation"), + cl::init(false), cl::cat(mainCategory)); + static cl::opt shouldMakeLUTs("lookup-tables", cl::desc("Optimize arcs into lookup tables"), cl::init(true), @@ -236,6 +246,12 @@ 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) @@ -243,12 +259,6 @@ static void populatePipeline(PassManager &pm) { 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