SystemVerilog block labels #585

lucasphillips · 2025-03-08T02:08:03Z

Description & Motivation

For ROHD classes which use begin/end to define blocks in generated SystemVerilog, support tagging the block with an optional label.

This is helpful to generate SystemVerilog which passes certain strict linting checks, and for compatibility with compilers which require labeled blocks.

Related Issue(s)

#146

Testing

Added a new set of tests which generate different types of blocks with labels and compile/compare the generated SystemVerilog.

Backwards-compatibility

No. The new parameters are optional.

Documentation

Does the change require any updates to documentation? If so, where? Are they included?

Yes. Requires description of new parameters and explanation of the feature.

For ROHD classes which use begin/end to define blocks in generated SystemVerilog, support tagging the block with an optional label. Add tests for labeled blocks. Implements intel#146 Co-authored-by: Curtis Anderson <curtis.anderson@intel.com> Co-authored-by: Luke Phillips <lucas.phillips@intel.com> Signed-off-by: Andrew Capatina <andrew.capatina@intel.com> Signed-off-by: Curtis Anderson <curtis.anderson@intel.com> Signed-off-by: Luke Phillips <lucas.phillips@intel.com>

lucasphillips · 2025-03-08T02:09:36Z

Once we are confident that we have the right approach to implement this feature, we will need to add some general documentation explaining what it does. Right now the only documentation is the descriptions of the new parameters.

mkorbel1 · 2025-03-10T18:49:38Z

lib/src/modules/conditionals/always.dart

@@ -21,6 +21,9 @@ abstract class Always extends Module with SystemVerilog {
      UnmodifiableListView<Conditional>(_conditionals);
  List<Conditional> _conditionals;

+  /// Optional block label
+  final String? label;


I wonder if for the top-level always block, we can just use the name as the label?

If we decide that makes sense, should we change other places where we have "label" to "name" as well?

mkorbel1 · 2025-03-10T18:51:31Z

lib/src/modules/conditionals/always.dart


    var verilog = '';
    verilog += '//  $instanceName\n';
-    verilog += '${alwaysVerilogStatement(inputs)} begin\n';
+    verilog += '${alwaysVerilogStatement(inputs)} begin$blockLabel\n';


question: does SystemVerilog require that block labels are unique? within some scope? if so, it would add a little complexity (hopefully it does not require it!)

your testing seems to indicate it is not necessary to make them unique -- do you know if that's LRM-legal or just ok with iverilog? any lints we should check for?

IEEE 1800-2023 says the following regarding hierarchical names (which includes identifiers used in named blocks):

23.6 Hierarchical names

Every identifier in a SystemVerilog description shall have a unique hierarchical path name. The hierarchy of modules and the definition of items such as tasks and named blocks within the modules shall define these names. The hierarchy of names can be viewed as a tree structure, where each module instance, generate block instance, task,
function, or named begin-end or fork-join block defines a new hierarchical level, or scope, in a particular branch of the tree.

... (snip) ...

Each node in the hierarchical name tree shall be a separate scope with respect to identifiers. A particular identifier can be declared at most once in any scope. See 23.9 for a discussion of scope rules and 3.13 for a discussion of name spaces.

... (continues)

And block names themselves are described elsewhere:

9.3.4 Block names

Both sequential and parallel blocks can be named by adding : name_of_block after the keywords begin or fork. A named block creates a new hierarchy scope. The naming of blocks serves the following purposes:

It allows local variables, parameters, and named events to be referenced hierarchically, using the block name.

It allows the block to be referenced in statements such as the disable statement (see 9.6.2).

An unnamed block creates a new hierarchy scope only if it directly contains a block item declaration, such as a variable declaration or a type declaration. This hierarchy scope is unnamed and the items declared in it cannot be hierarchically referenced (see 6.21).

All variables shall be static; that is, a unique location exists for all variables, and leaving or entering blocks shall not affect the values stored in them.

The block names give a means of uniquely identifying all variables at any simulation time.

A matching block name may be specified after the block end, join, join_any, or join_none keyword, preceded by a colon. This can help document which end or join, join_any, or join_none is associated with which begin or fork when there are nested blocks. A name at the end of the block is not required. It shall be an error if the name at the end is different from the block name at the beginning.

begin: blockB // block name after the begin or fork ... end: blockB

thanks for finding those sections -- my read of this is that they do need to be unique? That would, unfortunately, mean we need to probably uniquify them during SV generation in the Synthesizer stack. We'll have to think about whether that introduces a non-backwards-compatible change or if we can avoid it, as well. We could also experiment with violating the rule in multiple simulators and linters and seeing if our interpretation is correct or not.

mkorbel1 · 2025-03-10T18:52:38Z

lib/src/modules/conditionals/always.dart

@@ -105,8 +111,10 @@ abstract class Always extends Module with SystemVerilog {
          reset,
          // then use it for assigning receiver
          then: allResetCondAssigns,
+          ifLabel: '${label}_if',


since these two are specific to reset handling, perhaps naming them as such would be better? ${name]_reset and ${name}_main or something?

mkorbel1 · 2025-03-10T20:14:07Z

lib/src/modules/conditionals/case.dart

@@ -119,6 +127,9 @@ class Case extends Conditional {
  List<Conditional>? get defaultItem => _defaultItem;
  List<Conditional>? _defaultItem;

+  /// Optional label for the default case block.
+  final String? defaultLabel;


idea: what if we accepted a name instead, and used it to construct the default label, something like default_$name? not sure if that's better or worse. thoughts?

mkorbel1 · 2025-03-10T20:18:18Z

lib/src/modules/conditionals/flop.dart

@@ -146,7 +152,9 @@ class FlipFlop extends Module with SystemVerilog {
    var contents = [q < _d];

    if (_en != null) {
-      contents = [If(_en!, then: contents)];
+      contents = [
+        If(_en!, then: contents, ifLabel: label == null ? null : '${label}_en')


since flop uses custom instantiationVerilog, i think this label addition here actually has no effect. there's no begin/end blocks at all for this one-liner in generated SV.

mkorbel1 · 2025-03-10T20:19:41Z

lib/src/modules/conditionals/if.dart

+  If(Logic condition,
+      {List<Conditional>? then,
+      List<Conditional>? orElse,
+      String? ifLabel,


similar to prior comments: what if we just accepted one label/name and named the blocks if_$name and else_$name or something automatically? or do you think there's enough demand likely for people to customize the if and else block names separately? and if so, maybe it's ok to expect them to use the more verbose version of If.block?

mkorbel1 · 2025-03-10T20:25:13Z

test/block_labels_test.dart

+  }
+}
+
+void main() {


we should add some tests that actually generate SV and confirm some substring is inside, e.g.

final sv = mod.generateSynth(); expect(sv, contains('if(thing) : my_label');

mkorbel1 · 2025-03-10T20:26:45Z

test/block_labels_test.dart

+  Logic get out_0 => output('out_0');
+  Logic get out_1 => output('out_1');
+
+  String firstBlockLabel;


we should test that when no label is provided, the output SV looks right. could make these String? (nullable) and have some tests which set them to null, then confirm compilation and correct verilog generation (you could even loop through and run all your tests with both a string and null provided, if you want)

mkorbel1 linked an issue Mar 8, 2025 that may be closed by this pull request

Support tagging of generated SystemVerilog begin and end #146

Open

mkorbel1 reviewed Mar 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SystemVerilog block labels #585

SystemVerilog block labels #585

lucasphillips commented Mar 8, 2025

lucasphillips commented Mar 8, 2025

mkorbel1 Mar 10, 2025

mkorbel1 Mar 10, 2025

curtisan-intel Mar 19, 2025

mkorbel1 Mar 20, 2025

mkorbel1 Mar 10, 2025

mkorbel1 Mar 10, 2025

mkorbel1 Mar 10, 2025

mkorbel1 Mar 10, 2025

mkorbel1 Mar 10, 2025

mkorbel1 Mar 10, 2025

SystemVerilog block labels #585

Are you sure you want to change the base?

SystemVerilog block labels #585

Conversation

lucasphillips commented Mar 8, 2025

Description & Motivation

Related Issue(s)

Testing

Backwards-compatibility

Documentation

lucasphillips commented Mar 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment