ExportVerilog simplification for pretty-printing output verilog

TLDR; To make ExportVerilog less monolithic and to do pretty printing in the future, I think ExportVerilog would need some refactoring. As a first step, I’d like to move the AST legalization logic of ExportVerilog into PrepareForEmisson so that ExportVerilog doesn’t have to care about the legality of expressions.

Introduction

ExportVerilog is emitting production-quality verilog but one major missing feature is prettify-printing like clang-format or verible. Towards prettify-formating, I think we first need to fix the situation where ExportVerilog legalizes verilog expressions while emitting them. Let me first describe the current emission way.

hw.module @Foo(%a: i4, %b: i4) -> (c: i1) {
  %0 = comb.add %a, %b : i4
  %1 = comb.extract %0 from 3 : (i4) -> i1
  hw.output %1 : i1
}

ExportVerilog emits the following verilog:

  wire [3:0] _GEN = a + b;
  assign c = _GEN[3];	

%0 requires a temporary wire because (a+b)[3] is illegal in verilog. ExportVerilog dynamically checks the legality of expressions with isExpressionUnableToInline and spills a temporary wire if it is illegal. This procedure makes pretty-print difficult because it is hard to predict outputs from the IR. Hence, I want to perform the legalization in PrepareForEmission so that ExportVerilog doesn’t have to worry about legality, but can focus on prettify-printing of IR.

Proposal

Here, I’d like to describe how to legalize IR at PrepareForEmission.

Input:

hw.module @Foo(%a: i4, %b: i4) -> (c: i1) {
  %0 = comb.add %a, %b : i4
  %1 = comb.extract %0 from 3 : (i4) -> i1
  hw.output %1 : i1
}

After PrepareForEmission (expected):

hw.module @Foo(%a: i4, %b: i4) -> (c: i1) {
  %0 = comb.add %a, %b : i4
  
  // Explicitly create a wire op for a temporary wire
  %1 = sv.wire : !hw.inout<i4>
  sv.assign %1, %0 : i4
  %2 = sv.read_inout %1 : !hw.inout<i4>
  
  %3 = comb.extract %2 from 3 : (i4) -> i1
  hw.output %3 : i1
}

PrepareForEmission performs legalization by introducing a wire op for %0. sv.wire/sv.assign/sv.read_inout will be used here. One concern is that this legalization will create at least three operations per temporary wire so it might dramatically increase the size of IR.

ExportVerilog (now):

 wire [3:0] _GEN;
 assign _GEN = a + b;
 assign c = _GEN[3];

This is the current output for the IR above. Currently, wire ops are forwardly declared so it would be necessary to stop the forward declaration of wire ops.

ExportVerilog (expected):

 wire [3:0] _GEN = a + b;
 assign c = _GEN[3];

Once we can stop forward declarations of wire ops, the output should be exactly the same as now.

Plan

  1. Add spillTemporaryAtPrepare to the lowering option to selectively enable the new lowering.
  2. Expose isExpessionUnableToInline to PrepareForEmission and call isExpressionUnableToInline from shouldSpillWire in Prepare.
  3. Remove outOfLineExpressions from ModuleEmitter to stop unnecessary forward declaration of wire/regs. This part will require a lot of changes to ExportVerilog.
  4. Make spillTemporaryAtPrepare default and remove the option.

Any feedback and comment are highly appreciated!

3 Likes

Thanks for bringing this up. Big +1 from me for simplifying ExportVerilog.cpp and moving more to PrepareForExport.cpp. Some small comments inline.

I’d vote to push ahead on this route, but measure the increase in memory and runtime so we are clear what the impact is.

I believe there are some simple cases where the expression assigned to the wire is allowed inline rather than forward declared. I think it’s basically limited to constants now, but maybe after moving the legalization to Prepare, that can be extended to support more complex expressions. I think that happens here: circt/ExportVerilog.cpp at 33a4b17b5a26d18d30cd17c2929684026c5e4d5b · llvm/circt · GitHub

I look forward to the day that outOfLineExpressions doesn’t exist! Agreed this is the painful part.

I think it was implied, but would there be some time between step 3 and step 4? That might help test on more and more designs before we commit to the new approach.

Thank you for the reference! I agree we can extend this logic.

Right, I agree a lot of tests are necessary for migration. I also expect there should not be much difference in output verilog because this simplification must be almost a non-functional change.

Awesome, I’m on board with your plan, thanks for the clarifications!

I’m super onboard with this! Thank you for tackling it.

If the memory/performance around creating 3x operations for this is an issue, we could consider introducing a new type of wire operation to SV that avoids the need for the forward declaration and the need for the inout read (similar to a FIRRTL node). E.g.,:

%1 = sv.wire.inline %2 : !i4

This is clearly deviating from “SystemVerilog”, but I think it’s in the spirit of making the SystemVerilog dialect the most useful dialect for SystemVerilog emission which is its primary use.

More generally, I’m not against the introduction of operations that make emission more straightforward for the actual emission step. I.e., even with the sv.wire/sv.assign/sv.read_inout there is some annoyingly necessary pattern matching that needs to happen later. If an op makes this cleaner, I’m in favor.

1 Like

Yeah, I am actually in flavor of introducing this kind of operation. I agree this is redundant but I think it makes emission logic more straight forward. Probably it’s worth considering at some point.

1 Like

This makes sense to me too. Just to clarify, you’re not splitting these into distinct Passes, you’re just moving more into the prepare phase of the existing Pass, right?

If you’re splitting these into different passes, then we need to understand what the printer’s behavior is when the prepare pass isn’t run and something like this gets fed into it:

It is totally fine to produce an error, but isn’t fine to crash. If these remain one MLIR Pass then that shouldn’t be possible though!

Thanks for working on this!

-Chris

Hi Chris, thank you for the comment!

Yes, I’m not planning to split them into distinct passes at this point :slight_smile:
ExoprtVerilog internally runs PrepareForEmission before printing phase as it is now.