[ImportVerilog] Add $display/$write/$info/$warning/$error/$fatal #7642

fabianschuiki · 2024-09-27T05:20:50Z

Add support for the $display, $write, $info, $warning, $error, and $fatal system tasks.

These tasks are pretty complicated since they involve string formatting. Verilog has quite a few format specifiers and very strange rules for them. Luckily, Slang contains a handy utility that parses format strings and delegates handling of arguments and intermittent pieces of text to callbacks.

I've decided to follow the same IR design as the printing op in the Sim dialect: each format specifier is turned into a dedicated op that captures all relevant format options, plus additional literals for the text in between interpolated values, and concatenate everything into a single !moore.format_string. (Shoutout to @fzi-hielscher for trailblazing the nice design for sim.print!) This is handled in a new FormatStrings.cpp file inside of ImportVerilog.

The actual system tasks are mapped to new moore.builtin.display and moore.builtin.severity ops. These handle only the printing of the message in question, plus potential error bookkeeping. The $fatal system task creates additional moore.builtin.finish_message and moore.builtin.finish ops to represent its implicit call to $finish.

The implementation also handles the strange $displayb, $displayo, $displayh, $writeb, $writeo, and $writeh flavors of the tasks, where the suffix indicates the default format to use for arguments that are not covered by a format string literal. SystemVerilog is weird.

Thanks @hailongSun2000 for your prior work on this!

fabianschuiki · 2024-09-27T05:21:46Z

@hailongSun2000 I've based this on your #7600 PR 😃

terapines-osc-circt

I have reviewed the whole PR and understood the logical relationship. In terms of the functional logic, I approve. However, it's difficult that if I were to implement it. Therefore, I think maybe we can wait for another person to review it again. WDYT @fabianschuiki 😃?

terapines-osc-circt · 2024-09-27T06:10:11Z

include/circt/Dialect/Moore/MooreOps.td

+def FmtHexL : I32EnumAttrCase<"HexLower", 3, "hex_lower">;
+def FmtHexU : I32EnumAttrCase<"HexUpper", 4, "hex_upper">;
+def IntFormatAttr : I32EnumAttr<"IntFormat", "Integer format",
+                               [FmtDec, FmtBin, FmtOct, FmtHexL, FmtHexU]> {


If I don't misunderstand, the hex_lower is 0xFFF, and the hex_upper is 0xfff, right?

The exact opposite 😁. hex_lower gives you lowercase letters, fff, and hex_upper gives you uppercase letters, FFF.

Can you convince me that uppercase hex formatting actually exists in SV? Table 21-2 in the spec sounds like %x, %h, %X, %H are interchangeable. Testing it on some simulators, they all print lowercase only. And if Dave Rich hasn't seen it, I'm more inclined to believe in the existence of unicorns. 😉

🤣 Huh interesting, I could have sworn I've seen this print uppercase hexadecimal in VCS and Questa a while back 🤔 I might be wrong though.

lib/Conversion/ImportVerilog/Expressions.cpp

terapines-osc-circt · 2024-09-27T08:56:10Z

test/Conversion/ImportVerilog/basic.sv

+  // CHECK: [[TMP:%.+]] = moore.fmt.literal "\0A"
+  // CHECK: moore.builtin.display [[TMP]]


I'm curious why moore.fmt.literal can transfer \n into \0A 🤔?

The \n is parsed by Slang into the actual newline character (ASCII code 10, or 0x0A). This is then escaped again by MLIR's printer, which simply inserts the hexcodes for unprintable characters.

uenoku · 2024-09-27T14:47:09Z

lib/Conversion/ImportVerilog/FormatStrings.cpp

+                 << "string format specifier with width not supported";
+        emitLiteral(lit->getValue());
+        return success();
+      }


It only handles literal passed to format string arguments and doesn't accept a string variable like this, correct?

string s = "foo"; initial begin display("%s", s); end

Maybe it's more flexible if StringLiteralOp takes moore.string type SSA value and produces format string value but I think we can revisit this later 👍

Yeah totally! I started out with supporting only literals because they have a simpler special handling. Other cases we'd have to handle with something like moore.fmt.string which could take the different flavors of string that can be formatted. SystemVerilog is pretty weird: you can have a string type which we need to support, but unpacked byte arrays also count as strings, and string literals like "foo" themselves don't have string type, but rather generate a bit [N-1:0] array of appropriate length. There's also a whole bunch of craziness about taking arbtirary integers and interpreting them as right-aligned ASCII characters. 🤯

uenoku

LGTM! Not blocking but it would be great if in the future we can refactor enum attributes (format, padding, align) to share with sim.fmt.

lib/Conversion/ImportVerilog/FormatStrings.cpp

uenoku · 2024-09-27T14:51:41Z

lib/Conversion/ImportVerilog/FormatStrings.cpp

+      if (failed(emitArgument(specifier, format.substr(offset, len), options)))
+        anyFailure = true;
+    };
+    auto onError = [&](auto, auto, auto, auto) {};


Does onError need to set anyFailure when invoked?

The Slang frontend already runs this parse function and reports any errors it encounters. When ImportVerilog gets to this point, all errors have already reported and compilation would have already been aborted. I'll add a comment, and an assertion inside onError to check that this is correct. Thanks for the pointer!

uenoku · 2024-09-27T14:57:29Z

lib/Conversion/ImportVerilog/Statements.cpp

+    // Check for a `$display` or `$write` prefix.
+    bool isDisplay = false;     // display or write
+    bool appendNewline = false; // display
+    StringRef name = subroutine.name;


nit: this name variable looks like generic but it cannot be used other than handling display/write due to mutation. To avoid confusion can we change the variable name or create a local scope for handling display/write?

Good point. It might be worthwhile factoring the different flavors of builtins out into separate functions, and calling them in sequence to try and match the subroutine 🤔. I'll just rename the variable for now.

fabianschuiki · 2024-09-27T18:23:24Z

@uenoku [...] it would be great if in the future we can refactor enum attributes (format, padding, align) to share with sim.fmt.

I totally agree! @maerhart has suggested that we could try to factor out the formatting into a separate fmt dialect, and use that from Moore, FIRRTL, Sim, and others.

@fzi-hielscher

Add support for the `$display`, `$write`, `$info`, `$warning`, `$error`, and `$fatal` system tasks. These tasks are pretty complicated since they involve string formatting. Verilog has quite a few format specifiers and very strange rules for them. Luckily, Slang contains a handy utility that parses format strings and delegates handling of arguments and intermittent pieces of text to callbacks. I've decided to follow the same IR design as the printing op in the Sim dialect: each format specifier is turned into a dedicated op that captures all relevant format options, plus additional literals for the text in between interpolated values, and concatenate everything into a single `!moore.format_string`. (Shoutout to @fzi-hielscher for trailblazing the nice design for `sim.print`!) This is handled in a new `FormatStrings.cpp` file inside of ImportVerilog. The actual system tasks are mapped to new `moore.builtin.display` and `moore.builtin.severity` ops. These handle only the printing of the message in question, plus potential error bookkeeping. The `$fatal` system task creates additional `moore.builtin.finish_message` and `moore.builtin.finish` ops to represent its implicit call to `$finish`. The implementation also handles the strange `$displayb`, `$displayo`, `$displayh`, `$writeb`, `$writeo`, and `$writeh` flavors of the tasks, where the suffix indicates the default format to use for arguments that are not covered by a format string literal. SystemVerilog is weird. Thanks @hailongSun2000 for your prior work on this! Co-authored-by: Hailong Sun <hailong.sun@terapines.com>

fabianschuiki added Moore ImportVerilog labels Sep 27, 2024

fabianschuiki requested review from uenoku, fzi-hielscher, maerhart and terapines-osc-circt September 27, 2024 05:20

terapines-osc-circt approved these changes Sep 27, 2024

View reviewed changes

uenoku reviewed Sep 27, 2024

View reviewed changes

uenoku approved these changes Sep 27, 2024

View reviewed changes

fabianschuiki force-pushed the fschuiki/moore-severity branch from 14ca0ea to 5ba7ad9 Compare September 27, 2024 18:38

fabianschuiki merged commit 224bc57 into main Sep 27, 2024
4 checks passed

fabianschuiki deleted the fschuiki/moore-severity branch September 27, 2024 19:30

		// CHECK: [[TMP:%.+]] = moore.fmt.literal "\0A"
		// CHECK: moore.builtin.display [[TMP]]

[ImportVerilog] Add $display/$write/$info/$warning/$error/$fatal #7642

[ImportVerilog] Add $display/$write/$info/$warning/$error/$fatal #7642

Uh oh!

Conversation

fabianschuiki commented Sep 27, 2024

Uh oh!

fabianschuiki commented Sep 27, 2024

Uh oh!

terapines-osc-circt 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!

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!

uenoku left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

fabianschuiki commented Sep 27, 2024

Uh oh!

Uh oh!

Uh oh!

uenoku left a comment •

edited

Loading