[Arcilator] Allow running verif.simulation ops #8233

fabianschuiki · 2025-02-12T20:25:51Z

Add the LowerVerifSimulations pass to the Arc dialect. This pass converts any verif.simulation ops in the input into a test harness module and a func.func that instantiates the module and applies the necessary stimulus to it using arc.sim.* ops.

Also add the pass to the arcilator pipeline. This allows the user to run any verif.simulation ops in the input design by specifying the --run --jit-entry=<simulation-name> command line options. We may want to streamline this in the future.

Also add an integration test which verifies an adder circuit and prints the simulation results.

uenoku · 2025-02-12T20:51:06Z

integration_test/arcilator/JIT/verif-simulation.mlir

+  %failure = seq.compreg %6, %clock reset %init, %false : i1
+  %6 = comb.or %failure, %5 : i1
+
+  // Print some statistics about the test.


Nice use of DPI!

maerhart

Really cool work! 😎

maerhart · 2025-02-12T22:22:58Z

lib/Dialect/Arc/Transforms/LowerVerifSimulations.cpp

+        getOperation().getLoc(), builder.getStringAttr("exit"),
+        builder.getFunctionType({builder.getI32Type()}, {}));
+    SymbolTable::setSymbolVisibility(func, SymbolTable::Visibility::Private);
+  }


Don't we need to check the type of the existing function in the else case?

maerhart · 2025-02-12T22:25:20Z

lib/Dialect/Arc/Transforms/LowerVerifSimulations.cpp

+
+  SmallVector<StringAttr> symbols;
+  getOperation().walk([&](verif::SimulationOp op) {
+    auto *symbolTableOp = SymbolTable::getNearestSymbolTable(op);


Is there a need to have verif.simulation nested in arbitrary ops or could we require builtin.module as parent to simplify this?

maerhart · 2025-02-12T22:26:55Z

lib/Dialect/Arc/Transforms/LowerVerifSimulations.cpp

+    auto *symbolTableOp = SymbolTable::getNearestSymbolTable(op);
+    assert(symbolTableOp);
+    if (symbolTableOp == getOperation())
+      symbols.push_back(op.getSymNameAttr());


Why are you collecting the symbols here but never using them?

maerhart · 2025-02-12T22:39:35Z

lib/Dialect/Arc/Transforms/LowerVerifSimulations.cpp

+  auto &funcBody = funcOp.getBody().emplaceBlock();
+  builder.setInsertionPointToEnd(&funcBody);


Super-nit: builder has a method createBlock that does this in one line

maerhart · 2025-02-12T22:40:50Z

lib/Dialect/Arc/Transforms/LowerVerifSimulations.cpp

+  auto &instBody = instOp.getBody().emplaceBlock();
+  auto instArg = instBody.addArgument(instType, op.getLoc());
+  builder.setInsertionPointToEnd(&instBody);


maerhart · 2025-02-12T22:42:47Z

lib/Dialect/Arc/Transforms/LowerVerifSimulations.cpp

+  builder.create<SimSetInputOp>(op.getLoc(), instArg, clockName, lowOp);
+  builder.create<SimSetInputOp>(op.getLoc(), instArg, initName, trueOp);
+  builder.create<SimStepOp>(op.getLoc(), instArg);
+  builder.create<SimSetInputOp>(op.getLoc(), instArg, clockName, highOp);
+  builder.create<SimStepOp>(op.getLoc(), instArg);
+  builder.create<SimSetInputOp>(op.getLoc(), instArg, clockName, lowOp);
+  builder.create<SimSetInputOp>(op.getLoc(), instArg, initName, falseOp);
+  builder.create<SimStepOp>(op.getLoc(), instArg);


Nit: might be worth storing the location in a temporary

maerhart · 2025-02-12T23:00:42Z

lib/Dialect/Arc/Transforms/LowerVerifSimulations.cpp

+  // Compute `exit_code | (exit_code != 0)` as a way of guaranteeing that
+  // the exit code will be non-zero even if bits are truncated by the operating
+  // system.


Smart way to deal with the fact that exit typically ANDs with 0xFF. Maybe worth mentioning in the simulation op documentation that on non-zero exit code the least significant bit is always set? But I guess also not really, because we are very vague about that anyway.

maerhart · 2025-02-12T23:07:01Z

test/Dialect/Arc/lower-verif-simulations.mlir

+// CHECK:     arc.sim.set_input [[A]], "clock" = [[C0]]
+// CHECK:     arc.sim.set_input [[A]], "init" = [[I1]]
+// CHECK:     arc.sim.step [[A]]
+// CHECK:     arc.sim.set_input [[A]], "clock" = [[C1]]
+// CHECK:     arc.sim.step [[A]]
+// CHECK:     arc.sim.set_input [[A]], "clock" = [[C0]]
+// CHECK:     arc.sim.set_input [[A]], "init" = [[I0]]
+// CHECK:     arc.sim.step [[A]]


I think the simulation op description didn't mention that done is not checked when init=1. Which, of course, makes sense, but might be worth mentioning.

maerhart · 2025-02-12T23:11:10Z

lib/Dialect/Arc/Transforms/LowerVerifSimulations.cpp

+
+#include "circt/Dialect/Arc/ArcOps.h"
+#include "circt/Dialect/Arc/ArcPasses.h"
+#include "circt/Dialect/Comb/CombOps.h"


maerhart · 2025-02-12T23:17:02Z

integration_test/arcilator/JIT/verif-simulation.mlir

+  %0 = comb.add %count, %c1_i19 : i19
+
+  // Generate inputs to the adder.
+  %1, %2 = func.call @generateAdderInputs(%count) : (i19) -> (i42, i42)


Strictly speaking this would need to be a module instantiation since the region describes hardware, right? And the function would need to use comb instead of arith.

Yeah that's a good point. This happens to survive the Arcilator pipeline unharmed. A DPI call would probably fit better here, to reflect what you would type up in some SV input.

Add the `LowerVerifSimulations` pass to the Arc dialect. This pass converts any `verif.simulation` ops in the input into a test harness module and a `func.func` that instantiates the module and applies the necessary stimulus to it using `arc.sim.*` ops. Also add the pass to the arcilator pipeline. This allows the user to run any `verif.simulation` ops in the input design by specifying the `--run --jit-entry=<simulation-name>` command line options. We may want to streamline this in the future. Also add an integration test which verifies an adder circuit and prints the simulation results.

fabianschuiki added Arc Involving the `arc` dialect verif labels Feb 12, 2025

fabianschuiki requested review from seldridge and leonardt February 12, 2025 20:25

fabianschuiki requested a review from maerhart as a code owner February 12, 2025 20:25

fabianschuiki force-pushed the fschuiki/arcilator-verif-simulation branch from 8a3a802 to 5a66259 Compare February 12, 2025 20:48

uenoku reviewed Feb 12, 2025

View reviewed changes

maerhart approved these changes Feb 12, 2025

View reviewed changes

fabianschuiki force-pushed the fschuiki/verif-simulation branch from 2ab5136 to d99eb7a Compare February 13, 2025 21:43

fabianschuiki force-pushed the fschuiki/arcilator-verif-simulation branch from 5a66259 to bdf9e04 Compare February 13, 2025 21:43

Base automatically changed from fschuiki/verif-simulation to main February 14, 2025 16:18

fabianschuiki force-pushed the fschuiki/arcilator-verif-simulation branch from bdf9e04 to 588a431 Compare February 14, 2025 16:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Arcilator] Allow running verif.simulation ops #8233

[Arcilator] Allow running verif.simulation ops #8233

fabianschuiki commented Feb 12, 2025

uenoku Feb 12, 2025

maerhart left a comment

maerhart Feb 12, 2025

maerhart Feb 12, 2025

maerhart Feb 12, 2025

maerhart Feb 12, 2025

maerhart Feb 12, 2025

maerhart Feb 12, 2025

maerhart Feb 12, 2025

maerhart Feb 12, 2025

maerhart Feb 12, 2025

maerhart Feb 12, 2025

fabianschuiki Feb 14, 2025

		auto &funcBody = funcOp.getBody().emplaceBlock();
		builder.setInsertionPointToEnd(&funcBody);

[Arcilator] Allow running verif.simulation ops #8233

Are you sure you want to change the base?

[Arcilator] Allow running verif.simulation ops #8233

Conversation

fabianschuiki commented Feb 12, 2025

Choose a reason for hiding this comment

maerhart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment