[Comb] Add comb.reverse operation with Verilog Export #8758

mafeguimaraes · 2025-07-21T23:17:02Z

This PR introduces a new operation to the Comb dialect: comb.reverse, which performs bitwise reversal (mirroring) of an integer value.
It also adds Verilog export support for this operation using SystemVerilog’s streaming operator {<<{}}.

Bit reversal is a common operation in hardware design, especially in signal processing and communication protocols.
Previously, reversing bits required generating many explicit assign statements.
This new operation makes the IR cleaner and enables direct export to compact Verilog syntax.

Example

Original Verilog:

assign out[0] = in[7];
assign out[1] = in[6];
assign out[2] = in[5];
assign out[3] = in[4];
assign out[4] = in[3];
assign out[5] = in[2];
assign out[6] = in[1];
assign out[7] = in[0];

MLIR
%rev = comb.reverse %in : (i8) -> i8

Verilog (exported)
assign rev = {<<{in}};

Implementation Details

The implementation is divided into three main parts:

Operation Definition (CombOps.td, CombOps.cpp):
- The Comb_ReverseOp is defined in CombOps.td with Pure and SameOperandsAndResultType traits.
- The C++ implementation in CombOps.cpp includes:
  - Constant Folding: Handles bit-reversal for constant operands at compile time.
  - Canonicalization Pattern: Simplifies comb.reverse(comb.reverse(x)) to x.
  - Implements verify() for operand/result checks and inferResultType() for result inference.
Verilog Lowering (ExportVerilog.cpp):
- A dedicated emitter case for comb::ReverseOp was added.
- Lowers the operation to SystemVerilog’s streaming operator {<<{in}}, generating compact and readable Verilog design.
Testing (reverse.mlir, comb-reverse.mlir):
- A new test file was created to verify:
  - Constant folding and canonicalization via -canonicalize.
  - Verilog emission via -export-verilog and FileCheck.

fabianschuiki

Very cool! Just a few nitpicks and ideas for simplifications.

fabianschuiki · 2025-07-22T16:59:56Z

include/circt/Dialect/Comb/Combinational.td

+
+  let hasFolder = 1;
+  let hasCanonicalizer = 1;
+  let hasVerifier = 1;


Since the verifier just returns success(), you can simplify the op by removing this hasVerifier line and removing the verify function implementation 🙂

fabianschuiki · 2025-07-22T17:03:36Z

lib/Conversion/ExportVerilog/ExportVerilog.cpp

+
+  if (auto reverseOp = dyn_cast_or_null<comb::ReverseOp>(exp.getDefiningOp())) {
+    ps << "{<<{";
+    emitSubExpr(reverseOp.getInput(), LowestPrecedence);
+    ps << "}}";
+    emittedExprs.insert(reverseOp);
+    return {Symbol, IsUnsigned};
+  }


It would be fantastic if you could move this emission into a separate ExprEmitter::visitComb function. We already have one such function per Comb operation. That will then properly insert sign casts, parenthesis, and other things around the streaming operator.

Since you are adding a Comb op, you'll also have to add the op to the Comb op visitor. The visitor is what calls the various visitComb functions. You'll need to do this in two places:

https://github.com/llvm/circt/blob/main/include/circt/Dialect/Comb/CombVisitors.h#L41

https://github.com/llvm/circt/blob/main/include/circt/Dialect/Comb/CombVisitors.h#L102-L106

fabianschuiki · 2025-07-22T17:04:13Z

lib/Dialect/Comb/CombOps.cpp

+mlir::LogicalResult comb::ReverseOp::verify() {
+  return mlir::success();
+}


Not needed 🙂

fabianschuiki · 2025-07-22T17:08:58Z

lib/Dialect/Comb/CombOps.cpp

+void comb::ReverseOp::inferResultRanges(
+    llvm::ArrayRef<mlir::ConstantIntRanges> operandRanges,
+    llvm::function_ref<void(mlir::Value, const mlir::ConstantIntRanges &)>
+        setResultRangeFn) {
+  setResultRangeFn(getResult(), operandRanges.front());
+}


I'm not sure if this implementation of inferResultRanges is correct. As far as I understand it, that interface is supposed to compute upper and lower bounds for the integer value of the result. Since the ReverseOp reverses the bits of a number and therefore changes its value, the upper and lower bounds of the result are not the same as that of the input. The bit reversal probably has the annoying effect of splitting a contiguous range of integer values into potentially multiple ones.

Do any of your passes rely on having the InferIntRangeInterface implemented on this op? If not, I would delete this implementation and remove the DeclareOpInterfaceMethods<InferIntRangeInterface, ["inferResultRanges"]> trait from the op itself.

fabianschuiki · 2025-07-22T17:10:50Z

test/Conversion/ExportVerilog/comb-reverse.mlir

+// CHECK-LABEL: module test_reverse(
+// CHECK-NEXT:    input  [7:0] in,
+// CHECK-NEXT:    output [7:0] out
+// CHECK-NEXT:  );
+// CHECK-EMPTY:
+// CHECK-NEXT:    assign out = {<<{in}};
+// CHECK-NEXT:  endmodule
+
+hw.module @test_reverse(in %in: i8, out out: i8) {
+  %reversed = comb.reverse %in : (i8) -> i8
+  hw.output %reversed : i8
+}


Nice! Could you move this into https://github.com/llvm/circt/blob/main/test/Conversion/ExportVerilog/verilog-basic.mlir? That's where the other Comb operations are tested 🙂

fabianschuiki · 2025-07-22T17:11:32Z

test/Dialect/Comb/reverse.mlir

+// This test checks two things for comb.reverse:
+// 1. Constant folding: reversing a constant should be done at compile time.
+// 2. Canonicalization: the pattern reverse(reverse(x)) should be simplified to x.
+
+// CHECK-LABEL: hw.module @test_reverse_canonicalize
+// CHECK-SAME: (in %[[IN:.*]] : i8, out out_double_rev : i8, out out_const_rev : i8)
+
+// After canonicalization, there should be NO comb.reverse operations,
+// since reverse(reverse(x)) simplifies to x, and reverse(const) should be folded.
+// Therefore, we ensure there are no comb.reverse ops in the output:
+// CHECK-NOT: comb.reverse
+
+hw.module @test_reverse_canonicalize(in %in : i8, out out_double_rev : i8, out out_const_rev : i8) {
+
+  // Apply reverse twice to test canonicalization
+  %rev1 = comb.reverse %in : (i8) -> i8
+  %rev2 = comb.reverse %rev1 : (i8) -> i8
+
+  // Apply reverse on a constant to test folding
+  %c13 = hw.constant 13 : i8
+  %rev_const = comb.reverse %c13 : (i8) -> i8
+
+  // Check outputs: after canonicalization,
+  // out_double_rev should be wired directly to %in,
+  // and out_const_rev should be wired to the folded constant (which is -80 in i8)
+  hw.output %rev2, %rev_const : i8, i8
+}
+
+// CHECK: %[[CST:.*]] = hw.constant -80 : i8
+// CHECK: hw.output %[[IN]], %[[CST]] : i8, i8


Could you move this into https://github.com/llvm/circt/blob/main/test/Dialect/Comb/canonicalization.mlir? That's where the Comb dialect canonicalization tests are located 🙂

uenoku · 2025-07-22T18:34:14Z

include/circt/Dialect/Comb/Combinational.td

+  let hasCanonicalizer = 1;
+  let hasVerifier = 1;
+
+  let assemblyFormat = "$input attr-dict `:` functional-type($input, $result)";


I think functional-type is not necessary as it has SameOperandsAndResultType

Good point, you should be able to just use

`:` type($input)

here.

mafeguimaraes · 2025-07-23T11:18:06Z

Thank you both, @fabianschuiki and @uenoku, for the helpful review. I've implemented all the suggested changes. Please let me know if there is anything else. 🙂

fabianschuiki · 2025-07-23T16:07:18Z

Hey @mafeguimaraes, thanks a lot! It looks like there are a few clang-format issues that CI isn't happy about. These are just code formatting guidelines. Check out "clang format patches display" in https://github.com/llvm/circt/actions/runs/16469136872/job/46573783262?pr=8758 for details.

The other CI failure seems to be related to an accidental LLVM downgrade in your branch. Could you undo the LLVM submodule change in your branch such that it lines up again with what's on "main"? That should clear up the CI build.

mafeguimaraes · 2025-07-23T16:18:42Z

I've run clang-format to fix the style issues and reset the LLVM submodule to match main. Thank you, @fabianschuiki!

fabianschuiki · 2025-07-23T16:47:59Z

Hey @mafeguimaraes, it looks like the LLVM submodule is still pointing to a different commit than what's on main, causing CI to break. Try something like the following to change the submodule to point to the same commit as is on main:

git checkout main -- llvm

darthscsi

Thanks! a couple minor nits, but looking good.

darthscsi · 2025-07-23T16:45:07Z

lib/Conversion/ExportVerilog/ExportVerilog.cpp

@@ -2598,6 +2599,7 @@ SubExprInfo ExprEmitter::emitSubExpr(Value exp,
                                     SubExprSignRequirement signRequirement,
                                     bool isSelfDeterminedUnsignedValue,
                                     bool isAssignmentLikeContext) {
+


nit: stray whitespace change

darthscsi · 2025-07-23T16:48:45Z

lib/Conversion/ExportVerilog/ExportVerilog.cpp

+    emitError(op, "SV attributes emission is unimplemented for the op");
+
+  ps << "{<<{";
+  emitSubExpr(op.getInput(), LowestPrecedence);


It's been a while since I dealt with this, but is LowestPrecedence needed? The expression is insides brackets. It might be worth checking the spec to avoid unnecessary parens being emitted.

You are right, since the expression is inside the {<<{...}} streaming operator, it's in a new context and will never need to be parenthesized.

In this emitter, passing LowestPrecedence to the recursive emitSubExpr call is the idiomatic way to achieve exactly that and prevent any unnecessary parens from being added.

darthscsi · 2025-07-23T17:01:16Z

lib/Conversion/ExportVerilog/ExportVerilog.cpp

+  emitSubExpr(op.getInput(), LowestPrecedence);
+  ps << "}}";
+
+  return {Symbol, IsUnsigned};


Should this be concatenation precedence?

The streaming operator {<<{...}} should indeed have the same precedence as a regular concatenation.

darthscsi · 2025-07-23T17:01:43Z

lib/Conversion/ExportVerilog/ExportVerilog.cpp

+  emitSubExpr(op.getInput(), LowestPrecedence);
+  ps << "}}";
+
+  return {Symbol, IsUnsigned};


I'm having a hard time verifying unsigned results from the spec. It seems true, but I can't find it.

Yes, I had the same doubt. From what I found, the SystemVerilog streaming operator ({<<{}}) preserves the bit pattern without applying any sign extension or interpretation, which effectively makes the result unsigned.

fabianschuiki

LGTM! Very cool that we now have an op for this 😃

mafeguimaraes · 2025-07-23T18:20:01Z

That's great to hear! Thank you so much for the feedback and for guiding me through the process. I'm happy I could contribute. 😃

fabianschuiki · 2025-07-23T18:23:32Z

Looks like there's a minor breakage in an LLHD pass associated with the CombVisitor. I think you just need to create a visitComb stub for the ReverseOp here: https://github.com/llvm/circt/blob/main/lib/Dialect/LLHD/Transforms/DesequentializationPass.cpp#L155

Interesting that this didn't show up for you in your local build 🤔

mafeguimaraes · 2025-07-23T18:34:31Z

Looks like there's a minor breakage in an LLHD pass associated with the CombVisitor. I think you just need to create a visitComb stub for the ReverseOp here: https://github.com/llvm/circt/blob/main/lib/Dialect/LLHD/Transforms/DesequentializationPass.cpp#L155

Interesting that this didn't show up for you in your local build 🤔

Good catch! My local build configuration didn't include the LLHD dialect, so I missed this dependency. I've added the necessary visitComb stub for ReverseOp to the CombInterpreter. Thanks for pointing it out!

fabianschuiki · 2025-07-23T23:31:13Z

Looks like clang-format is unhappy about some trailing white space. Try running clang-format locally, or manually fix the whitespace 😃

fabianschuiki · 2025-07-24T15:03:45Z

@mafeguimaraes You can run the tests locally with ninja check-circt or make check-circt, depending on what build system you are using. That should allow you to debug the CI failures more quickly, since CI basically runs clang-format and ninja check-circt as well 🙂

mafeguimaraes and others added 10 commits July 10, 2025 10:40

Creating comb.reverse

8ed4580

Update Export Verilog with comb.reverse op

6bb4c13

Merge branch 'main' into comb-reverse-op

30c3cd0

update MooreToCore

88a94cc

update FlattenMemRefs

b2b0f7c

Merge branch 'llvm:main' into comb-reverse-op

e478810

update FlattenMemRefs

6285321

update FlattenMemRefs

299fdba

update FlattenMemRefs

cfe9327

update FlattenMemRefs

4f86967

mafeguimaraes requested a review from darthscsi as a code owner July 21, 2025 23:17

fabianschuiki reviewed Jul 22, 2025

View reviewed changes

uenoku reviewed Jul 22, 2025

View reviewed changes

Updating PR

609aa87

Adding clang-format

5c55a79

mafeguimaraes force-pushed the comb-reverse-op branch from ab48db1 to 5c55a79 Compare July 23, 2025 16:14

Merge branch 'llvm:main' into comb-reverse-op

90b7467

darthscsi approved these changes Jul 23, 2025

View reviewed changes

mafeguimaraes added 2 commits July 23, 2025 14:43

stray whitespace change

1a13b06

Update llvm submodule to match upstream/main

33b2b20

fabianschuiki approved these changes Jul 23, 2025

View reviewed changes

fix(LLHD): Add CombVisitor stub for comb.reverse

e41615f

mafeguimaraes requested a review from maerhart as a code owner July 23, 2025 18:35

fix(LLHD): Add CombVisitor stub for comb.reverse

1a44c39

running clang-format

e51303c

mafeguimaraes force-pushed the comb-reverse-op branch from fed503c to e51303c Compare July 24, 2025 11:25

mafeguimaraes and others added 2 commits July 24, 2025 08:26

Merge branch 'llvm:main' into comb-reverse-op

cb3a64e

Update verilog-basic test

9962d6f

[Comb] Add comb.reverse operation with Verilog Export #8758

Are you sure you want to change the base?

[Comb] Add comb.reverse operation with Verilog Export #8758

Uh oh!

Conversation

mafeguimaraes commented Jul 21, 2025

Example

Implementation Details

Uh oh!

fabianschuiki 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!

mafeguimaraes commented Jul 23, 2025

Uh oh!

fabianschuiki commented Jul 23, 2025

Uh oh!

mafeguimaraes commented Jul 23, 2025

Uh oh!

fabianschuiki commented Jul 23, 2025

Uh oh!

darthscsi 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!

fabianschuiki left a comment

Choose a reason for hiding this comment

Uh oh!

mafeguimaraes commented Jul 23, 2025

Uh oh!

fabianschuiki commented Jul 23, 2025

Uh oh!

mafeguimaraes commented Jul 23, 2025

Uh oh!

fabianschuiki commented Jul 23, 2025

Uh oh!

fabianschuiki commented Jul 24, 2025

Uh oh!

Uh oh!