[FIRRTL][Seq] Add explicit clock edge by Clo91eaf · Pull Request #10879 · llvm/circt

Clo91eaf · 2026-07-21T14:52:10Z

Add explicit positive-, negative-, and dual-edge clock attributes to FIRRTL registers and seq.firreg, preserving them through transformations and lowering.

Assisted-by: Codex:gpt-5.6-Sol

circt-bot · 2026-07-21T15:47:50Z

Results of circt-tests run for 2fefefa compared to results for f853892: no change to test results.

seldridge

Generally looks good. Thank you for getting this going!

High level: the optionality of the edgedness is unnecessary. It is fine to have a default valued attribute, though. However, you can also make a case that the default valued attribute is not really that useful over just making this mandatory and unset.

seldridge · 2026-07-22T03:00:09Z

+// Register clock edge. Mirrors the FIRRTL/SV event-control concept so that a
+// non-default (negedge or dual-edge) register clock survives lowering to SV.
+def ClockEdgePos : I32EnumAttrCase<"Pos", 0, "pos">;
+def ClockEdgeNeg : I32EnumAttrCase<"Neg", 1, "neg">;
+def ClockEdgeBoth : I32EnumAttrCase<"Both", 2, "both">;
+def ClockEdgeAttr : I32EnumAttr<"ClockEdge", "register clock edge",
+                                [ClockEdgePos, ClockEdgeNeg, ClockEdgeBoth]>;


This is consistent with how we've modeled this in the past. It occurred to me that this might be more compact as BitEnumAttr which would then allow a more compact representation for "both" which is just Pos | Neg. This is probably future work.

Additionally, we're starting to accumulate these enums in multiple dialects. Maybe there is eventually a common home in core for them which is sufficient for other dialects to just use. Not blocking or something that needs to be figured out now.

seldridge · 2026-07-22T03:01:50Z

    Optional<I1>:$reset, Optional<AnyType>:$resetValue,
-    UnitAttr:$isAsync
+    UnitAttr:$isAsync,
+    OptionalAttr<ClockEdgeAttr>:$clockEdge


Same comment about optional. At this point, it's fine if this is mandatory. However, it's not a good design to make it optional and then assume a default.

seldridge · 2026-07-22T03:02:18Z

+  case EventControl::AtEdge:
+    return seq::ClockEdgeAttr::get(context, seq::ClockEdge::Both);
+  }
+  return {};


This looks unnecessary given that the case is fully covered.

seldridge · 2026-07-22T03:02:28Z

+  if (!edge)
+    return {};


In purging the optional this should go away.

seldridge · 2026-07-22T03:05:39Z

+    // The transitions emitted below model a posedge-clocked register. Reject a
+    // non-default clock edge rather than silently emit an incorrect model.
+    if (auto firReg = dyn_cast<seq::FirRegOp>(op)) {
+      if (firReg.getClockEdge().value_or(seq::ClockEdge::Pos) !=
+          seq::ClockEdge::Pos) {
+        op->emitError("non-posedge clock edges are not supported by the BTOR2 "
+                      "conversion");
+        return;
+      }
+    }


This should go away with mandatory edgedness.

seldridge · 2026-07-22T03:05:48Z

+    return sv::EventControl::AtEdge;
+  }
+  return sv::EventControl::AtPosEdge;
+}


Same comments about default.

seldridge · 2026-07-22T03:06:02Z

+        if (reg.getClockEdge().value_or(seq::ClockEdge::Pos) !=
+            seq::ClockEdge::Pos) {
+          reg.emitOpError("only positive-edge registers are currently "
+                          "supported");
+          return signalPassFailure();
+        }


Same comments about edgedness.

seldridge · 2026-07-22T03:22:04Z

This PR is very close to going the distance and adding a syntax. Do you want to do that now?

Alternatively, we can delay that for later. However, please propose some syntax. What do you think would make sense here?

I’d prefer to defer the textual FIRRTL syntax to a follow-up PR

My proposed syntax is an optional edge qualifier immediately before the clock expression:

reg r : UInt<8>, clock reg r : UInt<8>, posedge clock reg r : UInt<8>, negedge clock reg r : UInt<8>, edge clock regreset r : UInt<8>, negedge clock, reset, UInt<8>(0)

The omitted qualifier would continue to mean posedge

This sounds good to me. I'm fine to defer. I only wanted to make sure that there was a plan for this. Thank you.

circt-bot · 2026-07-22T13:26:28Z

Results of circt-tests run for 161c820 compared to results for f853892: no change to test results.

circt-bot · 2026-07-22T15:43:18Z

Results of circt-tests run for a95574b compared to results for f853892: no change to test results.

seldridge

This continues to shape up great. I'm good to defer the FIRRTL syntax changes until later. The proposed syntax looks good.

seldridge · 2026-07-22T17:16:57Z

This sounds good to me. I'm fine to defer. I only wanted to make sure that there was a plan for this. Thank you.

seldridge · 2026-07-22T17:17:54Z

+    auto newReg =
+        RegOp::create(*builder, field.type, op.getClockVal(), "",
+                      NameKindEnum::DroppableName, attrs, StringAttr{});
+    newReg.setClockEdgeAttr(op.getClockEdgeAttr());


Is this cleaner if done directly in the builder asopposed to two statements?

seldridge · 2026-07-22T17:19:18Z

+  if (isa<RegOp, RegResetOp>(op) &&
+      cast<EventControlAttr>(op->getAttr("clockEdge")).getValue() ==
+          EventControl::AtPosEdge)
+    elides.push_back("clockEdge");


Doing this specialization based on a runtime type check of the operation is weird. Can this instead be moved into the RegOp/RegResetOp printer/parser?

seldridge · 2026-07-22T17:20:18Z

 bool RegOfVecToMemPass::analyzeMemoryPattern(FirRegOp reg,
                                             MemoryPattern &pattern) {
  LLVM_DEBUG(llvm::dbgs() << "Analyzing register: " << reg << "\n");

+  // FirMem ports are positive-edge triggered.
+  if (reg.getClockEdge() != ClockEdge::Pos)
+    return false;
+


What are the implications of this?

seq.firmem does not carry the clockEdge attribute, so if we do not check it, the pass will convert the negedge-trigged into posedge-trigged memory, chaging the update timing semantic. Maybe we can add the clockEdge semantic in the seq.firmem later.

seldridge · 2026-07-22T17:21:24Z

+
+// -----
+
+hw.module @NegEdgeReg(in %clk : !seq.clock, in %arg0: i8) {
+  // expected-error @below {{only positive-edge registers are currently supported}}
+  %reg = seq.firreg %arg0 clock %clk {clockEdge = 1 : i32} : i8
+}


Thank you for including a test of this!

seldridge · 2026-07-22T17:23:12Z

+    // CHECK: %reg = firrtl.reg %clock {clockEdge = 1 : i32}
+    %reg = firrtl.regreset %clock, %zero, %dummy {clockEdge = 1 : i32} : !firrtl.clock, !firrtl.uint<1>, !firrtl.uint<1>, !firrtl.uint<1>


This printer/parser syntax isn't ideal. Can this instead switch to something that either: used an attribute with a real name:

%reg = firrtl.regreset %clock, %zero, %dummy {clockEdge = negedge} : !firrtl.clock, !firrtl.uint<1>, !firrtl.uint<1>, !firrtl.uint<1>

Or:

%reg = firrtl.regreset negedge %clock, %zero, %dummy : !firrtl.clock, !firrtl.uint<1>, !firrtl.uint<1>, !firrtl.uint<1>

The latter is what I would go with.

Agreed. Now it represented as:

firrtl.reg %clock firrtl.reg negedge %clock firrtl.regreset negedge %clock, %reset, %value

circt-bot · 2026-07-24T10:08:51Z

Results of circt-tests run for 0c31fe0 compared to results for f853892: no change to test results.

circt-bot · 2026-07-24T11:17:39Z

Results of circt-tests run for 72ee42a compared to results for f853892: no change to test results.

circt-bot · 2026-07-24T14:51:52Z

Results of circt-tests run for 3e8cf11 compared to results for f853892: no change to test results.

Clo91eaf · 2026-07-28T12:00:12Z

@seldridge cc

Clo91eaf requested review from darthscsi, fabianschuiki, maerhart and seldridge as code owners July 21, 2026 14:52

Clo91eaf changed the title ~~Firrtl reg clock edge~~ [FIRRTL][Seq] Add explicit clock edge Jul 21, 2026

seldridge reviewed Jul 22, 2026

View reviewed changes

Clo91eaf force-pushed the firrtl-reg-clock-edge branch 2 times, most recently from 86fc858 to 161c820 Compare July 22, 2026 12:31

Clo91eaf force-pushed the firrtl-reg-clock-edge branch from 161c820 to 874c78e Compare July 22, 2026 13:35

[Seq] Add explicit clock edge to firreg

c2b4786

Clo91eaf force-pushed the firrtl-reg-clock-edge branch from 874c78e to a95574b Compare July 22, 2026 14:22

[FIRRTL] Add explicit clock edge to registers

a95574b

Clo91eaf requested a review from seldridge July 22, 2026 14:39

seldridge reviewed Jul 22, 2026

View reviewed changes

Clo91eaf force-pushed the firrtl-reg-clock-edge branch from a95574b to 0c31fe0 Compare July 24, 2026 08:55

Clo91eaf force-pushed the firrtl-reg-clock-edge branch from 0c31fe0 to 72ee42a Compare July 24, 2026 10:13

Clo91eaf force-pushed the firrtl-reg-clock-edge branch from 72ee42a to 3e8cf11 Compare July 24, 2026 13:43

[FIRRTL] Add clock edge to register builders

3e8cf11

		// CHECK: %reg = firrtl.reg %clock {clockEdge = 1 : i32}
		%reg = firrtl.regreset %clock, %zero, %dummy {clockEdge = 1 : i32} : !firrtl.clock, !firrtl.uint<1>, !firrtl.uint<1>, !firrtl.uint<1>

Uh oh!

Conversation

Clo91eaf commented Jul 21, 2026

Uh oh!

circt-bot Bot commented Jul 21, 2026

Uh oh!

seldridge left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

circt-bot Bot commented Jul 22, 2026

Uh oh!

circt-bot Bot commented Jul 22, 2026

Uh oh!

seldridge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

circt-bot Bot commented Jul 24, 2026

Uh oh!

circt-bot Bot commented Jul 24, 2026

Uh oh!

circt-bot Bot commented Jul 24, 2026

Uh oh!

Clo91eaf commented Jul 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants