[Moore] Improve WaitEventOp, lower to LLHD #7518

fabianschuiki · 2024-08-15T02:03:25Z

Rework the moore.wait_event op to be able to accurately model the semantics of SystemVerilog's @... event control statements. The op now has a body region which is executed to detect a relevant change in one or more interesting values. A new moore.detect_event op serves as the mechanism to encode whether a posedge, negedge, both, or any change at all on a value should be detected as an event.

Based on this new moore.wait_event op we can properly convert most of the event control statements in ImportVerilog to a corresponding MLIR op. Delay control like #1ns is not handled yet.

In the MooreToCore conversion this new op allows us to properly generate llhd.wait operations at the right place that suspend process execution until an interesting event has occurred. This now also allows us to support almost all SystemVerilog processes in the lowering. The only missing ones are always_comb and always_latch which require an implicit llhd.wait to be inserted. @maerhart has a version of that lowering almost ready though.

This commit also adds an llhd.final op in order to be able to lower final procedures.

maerhart

This is the first part of my review, I'll finish it later 😅

maerhart · 2024-08-15T08:43:37Z

include/circt/Dialect/LLHD/IR/LLHDStructureOps.td

@@ -51,6 +51,32 @@ def ProcessOp : LLHDOp<"process", [
  let assemblyFormat = "attr-dict-with-keyword $body";
 }

+def FinalOp : LLHDOp<"final", [
+  NoRegionArguments,


Do you think it'd make sense to add the RecursiveMemoryEffects trait?

Yeah good idea!

The ProcessOp probably also wants this, right?

Yes, good point!

include/circt/Dialect/LLHD/IR/LLHDStructureOps.td

include/circt/Dialect/Moore/MooreOps.td

maerhart · 2024-08-15T08:56:35Z

include/circt/Dialect/Moore/MooreOps.td

-def EventOp : MooreOp<"wait_event", [
-  HasParent<"ProcedureOp">
+def WaitEventOp : MooreOp<"wait_event", [
+  RecursiveMemoryEffects,


Should this have a HasParent<"ProcedureOp">?

I don't think so, because you can write something like this:

task foo(ref logic x); @(posedge x); endtask module Bar; logic y; always foo(y); endmodule

which would probably lower to this:

func.func @foo(%x: !moore.ref<l1>) { moore.wait_event { %0 = moore.read %x : <l1> moore.detect_event posedge %0 : l1 } } hw.module @Bar() { %y = moore.variable : <l1> moore.procedure always { func.call @foo(%y) : (!moore.ref<l1>) -> () } }

I'm not entirely sure how we'd deal with this… probably some lowering pass on functions to make sure we can suspend in the middle of a function call 🤔

Ugh forgot about tasks 😞
That's a good question indeed.

maerhart · 2024-08-15T08:58:51Z

tools/circt-verilog/circt-verilog.cpp

+    // TODO: Enable the following once it not longer interferes with @(...)
+    // event control checks. The introduced dummy variables make the event
+    // control observe a static local variable that never changes, instead of
+    // observing a module-wide signal.


Is this due to mem2reg and not having something like the sample op we talked about?

Yes exactly. The current implementation of SimplifyProcedures introduces shadow variables for any global variable that it accesses. Since we now have regular Control Flow ops and basic blocks, and mem2reg is becoming more solid, we'll have to revisit this and figure out what exactly we want to do in this pass. Some form of mapping assignments of global variables to process outputs would be great.

Because we originally planned to inline the body of always_comb into the top level/module(hw.module). Since we need to ensure the variable has a correct value(After running SimplifyProcedures, we will run mem2reg to eliminate the shadow variables). However, we would like to lower moore.procedure into llhd.process for the time being, I think maybe we needn't SimplifyProcedures, WDYT 😁?

Yes that sounds fantastic! 😀 I'm pretty sure we can add SimplifyProcedures again in the future to optimize some procedures early on, but I totally agree with going to LLHD processes for now and doing optimizations there. 🥳

test/Conversion/MooreToCore/basic.mlir

maerhart · 2024-08-15T09:09:23Z

include/circt/Dialect/Moore/MooreOps.td

+  }];
+  let arguments = (ins
+    EdgeAttr:$edge,
+    UnpackedType:$input,


Should this be restricted to a one-bit type or should we add a test for multi-bit operands?

I think you can have multi-bit operands, and the edge detection truth table will apply per bit. If the result is non-zero you get an event. The current MooreToCore conversion might need a few extensions to work reliably with multi-bit operands. Maybe a follow-up PR?

Erroring out in MooreToCore for now sounds good to me.

maerhart · 2024-08-15T09:15:37Z

lib/Conversion/MooreToCore/MooreToCore.cpp

+    // the interesting signals.
+    rewriter.setInsertionPointToEnd(checkBlock);
+    if (!triggers.empty()) {
+      auto triggered = rewriter.createOrFold<comb::OrOp>(loc, triggers, true);


So, no short-circuiting?

I don't think there is short-circuiting between the events in SystemVerilog. Having it be an expression may make lowering a bit easier later 😃

Ah, good to know that there's no short-circuiting in SV. 👍

cepheus69 · 2024-08-15T09:25:03Z

It is excited to see its taking shape 👍.

hailongSun2000 · 2024-08-15T11:13:11Z

lib/Conversion/MooreToCore/MooreToCore.cpp

+      for (auto returnOp :
+           llvm::make_early_inc_range(body.getOps<ReturnOp>())) {
+        rewriter.setInsertionPoint(returnOp);
+        rewriter.replaceOpWithNewOp<llhd::HaltOp>(returnOp);
+      }


If I remember correctly, the return statement cannot be used in a(n) initial/final. Slang also detects it(return statement is only valid inside task and function blocks).

Yes you're right! The Moore dialect has its own moore.return operation, which is used to terminated moore.procedure ops. We should probably give it a better name… A return in Verilog is mapped to a func.return in MLIR, which is different from the moore.return inserted implicitly as a terminator for moore.procedures 😬

Oh! I mistook body.getOps<ReturnOp> as the return of function. Thanks for correcting me!

maerhart

And the rest of my review. Thanks for all these great improvements! 🎉

maerhart · 2024-08-16T09:11:17Z

lib/Conversion/MooreToCore/MooreToCore.cpp

+    SmallDenseSet<Value> alreadyObserved;
+    clonedOp.getBody().walk([&](Operation *operation) {
+      for (auto value : operation->getOperands()) {
+        if (value.getParentRegion() == &clonedOp.getBody())


If there are nested regions within the wait_event op region, we'd have to check if clonedOp.getBody() is an ancestor region of the value defining region.

Nice catch! I've changed this and added a test for it.

maerhart · 2024-08-16T09:16:32Z

lib/Conversion/MooreToCore/MooreToCore.cpp

+      auto triggered = rewriter.createOrFold<comb::OrOp>(loc, triggers, true);
+      rewriter.create<cf::CondBranchOp>(loc, triggered, resumeBlock, waitBlock);
+    } else {
+      rewriter.create<cf::BranchOp>(loc, waitBlock);


In this case, the resume block would have no predecessors. Do you think it's save to leave this to the conversion framework? We don't have a test for this case to be sure.

Good point, I have added the pattern I've seen failing before to the test with an empty wait_event. It looks like the dialect conversion uses the dominance information collected before the conversion to iterate through ops -- looks like making a block unreachable doesn't exclude it from the conversion.

maerhart · 2024-08-16T09:20:49Z

Resolves #7482

@maerhart

Rework the `moore.wait_event` op to be able to accurately model the semantics of SystemVerilog's `@...` event control statements. The op now has a body region which is executed to detect a relevant change in one or more interesting values. A new `moore.detect_event` op serves as the mechanism to encode whether a posedge, negedge, both, or any change at all on a value should be detected as an event. Based on this new `moore.wait_event` op we can properly convert most of the event control statements in `ImportVerilog` to a corresponding MLIR op. Delay control like `#1ns` is not handled yet. In the MooreToCore conversion this new op allows us to properly generate `llhd.wait` operations at the right place that suspend process execution until an interesting event has occurred. This now also allows us to support almost all SystemVerilog processes in the lowering. The only missing ones are `always_comb` and `always_latch` which require an implicit `llhd.wait` to be inserted. @maerhart has a version of that lowering almost ready though. This commit also adds an `llhd.final` op in order to be able to lower `final` procedures. Fixes #7482. Co-authored-by: Martin Erhart <maerhart@outlook.com>

fabianschuiki added LLHD Moore ImportVerilog labels Aug 15, 2024

fabianschuiki requested review from maerhart and hailongSun2000 August 15, 2024 02:03

fabianschuiki force-pushed the fschuiki/wait-event branch 2 times, most recently from e33b25b to c540888 Compare August 15, 2024 02:17

maerhart reviewed Aug 15, 2024

View reviewed changes

hailongSun2000 reviewed Aug 15, 2024

View reviewed changes

fabianschuiki force-pushed the fschuiki/wait-event branch 2 times, most recently from 8ffa907 to 3c8775a Compare August 15, 2024 18:48

maerhart approved these changes Aug 16, 2024

View reviewed changes

fabianschuiki force-pushed the fschuiki/wait-event branch from 3c8775a to e4d0efc Compare August 16, 2024 19:17

fabianschuiki merged commit d71e6e3 into main Aug 16, 2024
4 checks passed

fabianschuiki deleted the fschuiki/wait-event branch August 16, 2024 20:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Moore] Improve WaitEventOp, lower to LLHD #7518

[Moore] Improve WaitEventOp, lower to LLHD #7518

fabianschuiki commented Aug 15, 2024

maerhart left a comment

maerhart Aug 15, 2024

fabianschuiki Aug 15, 2024

fabianschuiki Aug 15, 2024

maerhart Aug 16, 2024

maerhart Aug 15, 2024

fabianschuiki Aug 15, 2024

maerhart Aug 16, 2024

maerhart Aug 15, 2024

fabianschuiki Aug 15, 2024

hailongSun2000 Aug 16, 2024

fabianschuiki Aug 16, 2024

maerhart Aug 15, 2024

fabianschuiki Aug 15, 2024

maerhart Aug 16, 2024

maerhart Aug 15, 2024

fabianschuiki Aug 15, 2024

maerhart Aug 16, 2024

cepheus69 commented Aug 15, 2024

hailongSun2000 Aug 15, 2024

fabianschuiki Aug 15, 2024

hailongSun2000 Aug 16, 2024

maerhart left a comment

maerhart Aug 16, 2024

fabianschuiki Aug 16, 2024

maerhart Aug 16, 2024

fabianschuiki Aug 16, 2024

maerhart commented Aug 16, 2024

[Moore] Improve WaitEventOp, lower to LLHD #7518

[Moore] Improve WaitEventOp, lower to LLHD #7518

Conversation

fabianschuiki commented Aug 15, 2024

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

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

cepheus69 commented Aug 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

maerhart commented Aug 16, 2024