[circt-opt] Segmentation fault during XOR op building

[likeamahoney]

Hi, all! Let's consider a such Verilog example:

module top (input clk, input rstn);
    reg r1;
    always @(posedge clk or negedge rstn) begin
        r1 = 0;
    end
endmodule

During llhd-desequentialize pass circt-opt tool segfaults using this pipeline:

circt-verilog test.sv | circt-opt --llhd-desequentialize

With such stacktrace:

PLEASE submit a bug report to https://github.com/llvm/circt and include the crash backtrace.
Stack dump:
0.	Program arguments: ./circt-opt --llhd-desequentialize
 #0 0x00005f1950d1c582 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (./circt-opt+0x19b1582)
 #1 0x00005f1950d19312 SignalHandler(int, siginfo_t*, void*) Signals.cpp:0:0
 #2 0x000073e5fec42520 (/lib/x86_64-linux-gnu/libc.so.6+0x42520)
 #3 0x00005f19517f1f60 circt::hw::isHWIntegerType(mlir::Type) (./circt-opt+0x2486f60)
 #4 0x00005f1950f4aa48 circt::comb::__mlir_ods_local_type_constraint_Comb1(mlir::Operation*, mlir::Type, llvm::StringRef, unsigned int) CombOps.cpp:0:0
 #5 0x00005f1950f861df circt::comb::XorOp::verifyInvariantsImpl() (./circt-opt+0x1c1b1df)
 #6 0x00005f1950fd06f8 mlir::Op<circt::comb::XorOp, mlir::OpTrait::ZeroRegions, mlir::OpTrait::OneResult, mlir::OpTrait::OneTypedResult<circt::hw::TypeVariant<mlir::IntegerType, circt::hw::IntType>>::Impl, mlir::OpTrait::ZeroSuccessors, mlir::OpTrait::VariadicOperands, mlir::OpTrait::OpInvariants, mlir::BytecodeOpInterface::Trait, mlir::OpTrait::IsCommutative, mlir::OpTrait::SameTypeOperands, mlir::OpTrait::SameOperandsAndResultType, mlir::InferIntRangeInterface::Trait, mlir::ConditionallySpeculatable::Trait, mlir::OpTrait::AlwaysSpeculatableImplTrait, mlir::MemoryEffectOpInterface::Trait>::verifyInvariants(mlir::Operation*) CombDialect.cpp:0:0
 #7 0x00005f1950fcff9e mlir::RegisteredOperationName::Model<circt::comb::XorOp>::verifyInvariants(mlir::Operation*) CombDialect.cpp:0:0
 #8 0x00005f1952540efa (anonymous namespace)::OperationVerifier::verifyOperation(mlir::Operation&) Verifier.cpp:0:0
 #9 0x00005f19525426e8 mlir::verify(mlir::Operation*, bool) (./circt-opt+0x31d76e8)
#10 0x00005f195318ea65 mlir::detail::OpToOpPassAdaptor::run(mlir::Pass*, mlir::Operation*, mlir::AnalysisManager, bool, unsigned int) (./circt-opt+0x3e23a65)
#11 0x00005f195318ec29 mlir::detail::OpToOpPassAdaptor::runPipeline(mlir::OpPassManager&, mlir::Operation*, mlir::AnalysisManager, bool, unsigned int, mlir::PassInstrumentor*, mlir::PassInstrumentation::PipelineParentInfo const*) (./circt-opt+0x3e23c29)
#12 0x00005f195318dc1e mlir::detail::OpToOpPassAdaptor::runOnOperationAsyncImpl(bool) (./circt-opt+0x3e22c1e)
#13 0x00005f195318e58f mlir::detail::OpToOpPassAdaptor::run(mlir::Pass*, mlir::Operation*, mlir::AnalysisManager, bool, unsigned int) (./circt-opt+0x3e2358f)
#14 0x00005f195318ec29 mlir::detail::OpToOpPassAdaptor::runPipeline(mlir::OpPassManager&, mlir::Operation*, mlir::AnalysisManager, bool, unsigned int, mlir::PassInstrumentor*, mlir::PassInstrumentation::PipelineParentInfo const*) (./circt-opt+0x3e23c29)
#15 0x00005f195318fc5b mlir::PassManager::run(mlir::Operation*) (./circt-opt+0x3e24c5b)
#16 0x00005f19529aa732 performActions(llvm::raw_ostream&, std::shared_ptr<llvm::SourceMgr> const&, mlir::MLIRContext*, mlir::MlirOptMainConfig const&) MlirOptMain.cpp:0:0
#17 0x00005f19529ac006 llvm::LogicalResult llvm::function_ref<llvm::LogicalResult (std::unique_ptr<llvm::MemoryBuffer, std::default_delete<llvm::MemoryBuffer>>, llvm::MemoryBufferRef const&, llvm::raw_ostream&)>::callback_fn<mlir::MlirOptMain(llvm::raw_ostream&, std::unique_ptr<llvm::MemoryBuffer, std::default_delete<llvm::MemoryBuffer>>, mlir::DialectRegistry&, mlir::MlirOptMainConfig const&)::'lambda'(std::unique_ptr<llvm::MemoryBuffer, std::default_delete<llvm::MemoryBuffer>>, llvm::MemoryBufferRef, llvm::raw_ostream&)>(long, std::unique_ptr<llvm::MemoryBuffer, std::default_delete<llvm::MemoryBuffer>>, llvm::MemoryBufferRef const&, llvm::raw_ostream&) MlirOptMain.cpp:0:0
#18 0x00005f19529e3476 mlir::splitAndProcessBuffer(std::unique_ptr<llvm::MemoryBuffer, std::default_delete<llvm::MemoryBuffer>>, llvm::function_ref<llvm::LogicalResult (std::unique_ptr<llvm::MemoryBuffer, std::default_delete<llvm::MemoryBuffer>>, llvm::MemoryBufferRef const&, llvm::raw_ostream&)>, llvm::raw_ostream&, llvm::StringRef, llvm::StringRef) (./circt-opt+0x3678476)
#19 0x00005f19529a963d mlir::MlirOptMain(llvm::raw_ostream&, std::unique_ptr<llvm::MemoryBuffer, std::default_delete<llvm::MemoryBuffer>>, mlir::DialectRegistry&, mlir::MlirOptMainConfig const&) (.part.0) MlirOptMain.cpp:0:0
#20 0x00005f19529ac6c8 mlir::MlirOptMain(int, char**, llvm::StringRef, llvm::StringRef, mlir::DialectRegistry&) (./circt-opt+0x36416c8)
#21 0x00005f19529ac8c2 mlir::MlirOptMain(int, char**, llvm::StringRef, mlir::DialectRegistry&) (./circt-opt+0x36418c2)
#22 0x00005f1950c53ff8 main (./circt-opt+0x18e8ff8)
#23 0x000073e5fec29d90 __libc_start_call_main ./csu/../sysdeps/nptl/libc_start_call_main.h:58:16
#24 0x000073e5fec29e40 call_init ./csu/../csu/libc-start.c:128:20
#25 0x000073e5fec29e40 __libc_start_main ./csu/../csu/libc-start.c:379:5
#26 0x00005f1950c53895 _start (./circt-opt+0x18e8895)

mlir generated by circt-verilog presented below:

module {
  hw.module @top(in %clk : i1, in %rstn : i1) {
    %true = hw.constant true
    %false = hw.constant false
    %0:2 = llhd.process -> i1, i1 {
      cf.br ^bb1(%clk, %rstn, %false, %false : i1, i1, i1, i1)
    ^bb1(%1: i1, %2: i1, %3: i1, %4: i1):  // 3 preds: ^bb0, ^bb2, ^bb2
      llhd.wait yield (%3, %4 : i1, i1), (%clk, %rstn : i1, i1), ^bb2(%1, %2 : i1, i1)
    ^bb2(%5: i1, %6: i1):  // pred: ^bb1
      %7 = comb.xor bin %5, %true : i1
      %8 = comb.and bin %7, %clk : i1
      %9 = comb.xor bin %rstn, %true : i1
      %10 = comb.and bin %6, %9 : i1
      %11 = comb.or bin %8, %10 : i1
      cf.cond_br %11, ^bb1(%clk, %rstn, %false, %true : i1, i1, i1, i1), ^bb1(%clk, %rstn, %false, %false : i1, i1, i1, i1)
    }
    hw.output
  }
}

[fzi-hielscher]

Hi @likeamahoney,

I hope @fabianschuiki or @maerhart will correct me if I'm wrong. As far as I know --llhd-desequentialize has been deprecated and replaced by --llhd-deseq which appears to be handling your example just fine. Is there a reason you are specifically invoking --llhd-desequentialize?

[likeamahoney]

Hi @likeamahoney,

I hope @fabianschuiki or @maerhart will correct me if I'm wrong. As far as I know --llhd-desequentialize has been deprecated and replaced by --llhd-deseq which appears to be handling your example just fine. Is there a reason you are specifically invoking --llhd-desequentialize?

Hi, I have't got any specific reason for running this pass instead of --llhd-deseq. I just tried some combinations of passes to lower my example to arc and llvm and during this have found it.

So If it is deprecated I can close this issue.

[fabianschuiki]

Nice find @likeamahoney! 😃 The fact that circt-verilog spits out an llhd.process means that the llhd-deseq pass ran as part of the circt-verilog pipeline didn't properly recognize this process as a register. That is definitely a bug. @fzi-hielscher is also right, the llhd-desequentialize pass is deprecated. We should just delete that at this point, since the pipeline has been running pretty well for a few months now with only the updated passes.

I think the thing that trips up the Deseq pass in your example is the fact that its using a blocking assignment r1 = 0 instead of a non-blocking assignment r1 <= 0 in the always process. Does this come from some real-world Verilog source code? The issue with a non-blocking assignment is that it would happen immediately when the always process is triggered by a clock edge, instead of the usual propagation delay of 1 delta time step that tries to emulate register propagation times in hardware. If you had multiple registers with = assignments that use each other's values, this would create a race condition where some registers would already see the updated value of other registers. The delta time step is necessary to give all always processes in the design a chance to trigger and read the old values of their input registers, and then have all register changes apply 1 delta time step worth of propagation delay later.

[likeamahoney]

Nice find @likeamahoney! 😃 The fact that circt-verilog spits out an llhd.process means that the llhd-deseq pass ran as part of the circt-verilog pipeline didn't properly recognize this process as a register. That is definitely a bug. @fzi-hielscher is also right, the llhd-desequentialize pass is deprecated. We should just delete that at this point, since the pipeline has been running pretty well for a few months now with only the updated passes.

I think the thing that trips up the Deseq pass in your example is the fact that its using a blocking assignment r1 = 0 instead of a non-blocking assignment r1 <= 0 in the always process. Does this come from some real-world Verilog source code? The issue with a non-blocking assignment is that it would happen immediately when the always process is triggered by a clock edge, instead of the usual propagation delay of 1 delta time step that tries to emulate register propagation times in hardware. If you had multiple registers with = assignments that use each other's values, this would create a race condition where some registers would already see the updated value of other registers. The delta time step is necessary to give all always processes in the design a chance to trigger and read the old values of their input registers, and then have all register changes apply 1 delta time step worth of propagation delay later.

Thank you for the answer!

I got this example from one of my previous issue -.

Yes blocking assignments it is not the best way to design sequential logic but such kind of examples are presence in real-world open and proprietary designs. For example famous picorv32 which was synthesed many times - https://github.com/YosysHQ/picorv32/blob/87c89acc18994c8cf9a2311e871818e87d304568/picorv32.v#L1406.

[fabianschuiki]

Nice, thanks! I'm wondering how we should deal with such inputs which are subtly wrong. There are a few other common Verilog design issues that circt-verilog already works around:

• treat always @* as always_comb, because people often don't understand that with always @* the process is not executed on the initial values of the input signals
• assume that a case ... endcase statement which covers all two-valued possibilities is meant to be covering all four-valued possibilities (this one is wild and wrong, but people rely on it in the real world)

We could think about treating = blocking assignments in an always process with edge sensitivity, e.g. posedge clk, as a non-blocking assignment for the sake of register detection. We should probably make that a lowering option, or something we detect and fixup as a pass at the Moore dialect level. The user should then be able to disable this fixup if they want accurate Verilog compliance. WDYT?