[FIRRTL] Register with Self Connection Crashes Exporter

seldridge commented 1 year ago

Consider the following FIRRTL Dialect:

firrtl.circuit "Foo" {
  firrtl.module @Foo(in %clock: !firrtl.clock, in %a: !firrtl.uint<1>, out %b: !firrtl.uint<1>) {
    %c1_ui1 = firrtl.constant 1 : !firrtl.uint<1>
    %0 = firrtl.wire : !firrtl.uint<1>
    %r = firrtl.regreset interesting_name %clock, %c1_ui1, %0 : !firrtl.clock, !firrtl.uint<1>, !firrtl.uint<1>, !firrtl.uint<1>
    firrtl.strictconnect %0, %r : !firrtl.uint<1>
  }
}

This produces a nice crash in the exporter (circt-translate -export-firrtl Bar.mlir):

Assertion failed: (!s->text().empty() && "empty string token"), function operator(), file PrettyPrinter.cpp, line 91.
PLEASE submit a bug report to https://github.com/llvm/circt and include the crash backtrace.
Stack dump:
0.      Program arguments: circt-translate -export-firrtl Bar.mlir
Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):
0  circt-translate          0x00000001079167ed llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) + 61
1  circt-translate          0x0000000107916d5b PrintStackTraceSignalHandler(void*) + 27
2  circt-translate          0x0000000107914896 llvm::sys::RunSignalHandlers() + 134
3  circt-translate          0x0000000107918b8f SignalHandler(int) + 223
4  libsystem_platform.dylib 0x00007ff81820c5ed _sigtramp + 29
5  libsystem_platform.dylib 0x00007ff7b877dd20 _sigtramp + 18446744072104646480
6  libsystem_c.dylib        0x00007ff818105b45 abort + 123
7  libsystem_c.dylib        0x00007ff818104e5e err + 0
8  circt-translate          0x0000000108746000 circt::pretty::PrettyPrinter::add(circt::pretty::Token)::$_1::operator()(circt::pretty::StringToken*) const + 176
9  circt-translate          0x0000000108745ee8 llvm::TypeSwitch<circt::pretty::Token*, void>& llvm::TypeSwitch<circt::pretty::Token*, void>::Case<circt::pretty::StringToken, circt::pretty::PrettyPrinter::add(circt::pretty::Token)::$_1>(circt::pretty::PrettyPrinter::add(circt::pretty::Token)::$_1&&) + 88
10 circt-translate          0x0000000108744af5 llvm::TypeSwitch<circt::pretty::Token*, void>& llvm::detail::TypeSwitchBase<llvm::TypeSwitch<circt::pretty::Token*, void>, circt::pretty::Token*>::Case<circt::pretty::PrettyPrinter::add(circt::pretty::Token)::$_1>(circt::pretty::PrettyPrinter::add(circt::pretty::Token)::$_1&&) + 53
11 circt-translate          0x0000000108744a03 circt::pretty::PrettyPrinter::add(circt::pretty::Token) + 99
12 circt-translate          0x000000010819d235 std::__1::enable_if<std::is_base_of_v<circt::pretty::Token, circt::pretty::StringToken>, void>::type circt::pretty::TokenBuilder<circt::pretty::PrettyPrinter>::add<circt::pretty::StringToken, llvm::StringRef&>(llvm::StringRef&) + 101
13 circt-translate          0x000000010819d1c1 circt::pretty::TokenBuilder<circt::pretty::PrettyPrinter>::literal(llvm::StringRef) + 33
14 circt-translate          0x000000010819c118 circt::pretty::TokenStream<circt::pretty::PrettyPrinter>::operator<<(circt::pretty::PPExtString const&) + 56
15 circt-translate          0x00000001081c2fa8 (anonymous namespace)::Emitter::emitStatement(circt::firrtl::WireOp)::$_5::operator()() const + 104
16 circt-translate          0x00000001081c2f35 decltype(static_cast<(anonymous namespace)::Emitter::emitStatement(circt::firrtl::WireOp)::$_5>(fp)()) std::__1::__invoke<(anonymous namespace)::Emitter::emitStatement(circt::firrtl::WireOp)::$_5>((anonymous namespace)::Emitter::emitStatement(circt::firrtl::WireOp)::$_5&&) + 21
17 circt-translate          0x00000001081c2e5d std::__1::invoke_result<(anonymous namespace)::Emitter::emitStatement(circt::firrtl::WireOp)::$_5>::type std::__1::invoke<(anonymous namespace)::Emitter::emitStatement(circt::firrtl::WireOp)::$_5>((anonymous namespace)::Emitter::emitStatement(circt::firrtl::WireOp)::$_5&&) + 29
18 circt-translate          0x00000001081c2d96 auto circt::pretty::TokenStream<circt::pretty::PrettyPrinter>::scopedBox<circt::pretty::PP, (anonymous namespace)::Emitter::emitStatement(circt::firrtl::WireOp)::$_5>(circt::pretty::PP&&, (anonymous namespace)::Emitter::emitStatement(circt::firrtl::WireOp)::$_5&&, circt::pretty::Token) + 102
19 circt-translate          0x00000001081c2d13 (anonymous namespace)::Emitter::emitStatement(circt::firrtl::WireOp) + 179
20 circt-translate          0x00000001081c2808 auto (anonymous namespace)::Emitter::emitStatementsInBlock(mlir::Block&)::$_16::operator()<circt::firrtl::WireOp>(circt::firrtl::WireOp) const + 40
21 circt-translate          0x00000001081c278b llvm::TypeSwitch<mlir::Operation*, void>& llvm::TypeSwitch<mlir::Operation*, void>::Case<circt::firrtl::WireOp, (anonymous namespace)::Emitter::emitStatementsInBlock(mlir::Block&)::$_16&>((anonymous namespace)::Emitter::emitStatementsInBlock(mlir::Block&)::$_16&) + 107
22 circt-translate          0x000000010819ea0d (anonymous namespace)::Emitter::emitStatementsInBlock(mlir::Block&) + 365
23 circt-translate          0x000000010819e669 (anonymous namespace)::Emitter::emitModule(circt::firrtl::FModuleOp)::$_1::operator()() const + 217

dtzSiFive commented 1 year ago

Oh no! FWIW before PrettyPrinter port, this produced the following (wrong) input, either way this should be fixed!

circuit Foo :
  module Foo :
    input clock : Clock
    input a : UInt<1>
    output b : UInt<1>

    wire  : UInt<1> @[./issue-5204.mlir 4:10]
    reg r : UInt<1>, clock with :
      reset => (UInt<1>(1), ) @[./issue-5204.mlir 5:10]
     <= r @[./issue-5204.mlir 6:5]

(from circt-translate 1.39.0g20230420_640e43b)

dtzSiFive commented 1 year ago

Looks like the wire is wrong too, and inspecting the emitter it seems to expect operations to already have names (and does this sort of problematic thing if they don't).

This variant of the original input works, for example (%0 -> %w):

firrtl.circuit "Foo" {
  firrtl.module @Foo(in %clock: !firrtl.clock, in %a: !firrtl.uint<1>, out %b: !firrtl.uint<1>) {
    %c1_ui1 = firrtl.constant 1 : !firrtl.uint<1>
    %w = firrtl.wire : !firrtl.uint<1>
    %r = firrtl.regreset interesting_name %clock, %c1_ui1, %w : !firrtl.clock, !firrtl.uint<1>, !firrtl.uint<1>, !firrtl.uint<1>
    firrtl.strictconnect %w, %r : !firrtl.uint<1>
  }
}

Maybe should add names before running emitter or teach it to generate names if none are present?

llvm / circt

[FIRRTL] Register with Self Connection Crashes Exporter #5204