Skip to content

SystemVerilog block labels #585

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

SystemVerilog block labels #585

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions lib/src/modules/conditionals/always.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ abstract class Always extends Module with SystemVerilog {
UnmodifiableListView<Conditional>(_conditionals);
List<Conditional> _conditionals;

/// Optional block label
final String? label;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


/// A mapping from internal receiver signals to designated [Module] outputs.
@protected
@internal
Expand All @@ -47,7 +50,10 @@ abstract class Always extends Module with SystemVerilog {
/// driven by any other [Conditional] in this block, it will be driven to the
/// specified reset value.
Always(this._conditionals,
{Logic? reset, Map<Logic, dynamic>? resetValues, super.name = 'always'}) {
{Logic? reset,
Map<Logic, dynamic>? resetValues,
super.name = 'always',
this.label}) {
// create a registration of all inputs and outputs of this module
var idx = 0;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// else assign zero as resetValue
orElse: conditionals,
elseLabel: '${label}_else',
),
];
}
Expand Down Expand Up @@ -176,12 +184,14 @@ abstract class Always extends Module with SystemVerilog {
ports.entries.where((element) => this.inputs.containsKey(element.key)));
final outputs = Map.fromEntries(ports.entries
.where((element) => this.outputs.containsKey(element.key)));
final blockLabel =
label == null ? '' : ' : ${Sanitizer.sanitizeSV(label!)}';

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so reading into it and thinking a little more, since each block scope is a new scope, the naming only needs to be uniquified locally within that scope! that means we can use the name for top-level always blocks, since that's already unique, then we can handle uniquification at Conditional levels each step through the verilogContents API, which is much less disruptive for uniquification! I think that works

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkorbel1 we can look into this, but maybe this is better as a different issue we can address? I'm not seeing a clean way of addressing this that doesn't make this an unwieldly PR. Not exactly familiar enough with how ROHD is generating blocks to say I have a good recursive solution for this. We could do a list and lookup for current scope but I think that is a substantial change to the code and would maybe make code review less reasonable (I'm also not a dart expert and still learning so I'm trying to keep my lack of current knowledge in mind).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can meet to sketch out what it looks like? i think once we figure out the pattern it will end up being relatively simple and not too disruptive to apply it

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, we would be happy too, we just want to get you unblocked.

verilog += _alwaysContents(inputs, outputs, assignOperator());
verilog += 'end\n';
verilog += 'end$blockLabel\n';
return verilog;
}
}
37 changes: 27 additions & 10 deletions lib/src/modules/conditionals/case.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import 'package:collection/collection.dart';
import 'package:meta/meta.dart';
import 'package:rohd/rohd.dart';
import 'package:rohd/src/modules/conditionals/ssa.dart';
import 'package:rohd/src/utilities/sanitizer.dart';

/// Represents a single case within a [Case] block.
class CaseItem {
Expand All @@ -20,8 +21,11 @@ class CaseItem {
/// A [List] of [Conditional]s to execute when [value] is matched.
final List<Conditional> then;

/// An optional label for this case body.
final String? label;

/// Executes [then] when [value] matches.
CaseItem(this.value, this.then);
CaseItem(this.value, this.then, {this.label});

@override
String toString() => '$value : $then';
Expand All @@ -39,7 +43,8 @@ class CaseItem {
Logic cases(Logic expression, Map<dynamic, dynamic> conditions,
{int? width,
ConditionalType conditionalType = ConditionalType.none,
dynamic defaultValue}) {
dynamic defaultValue,
String? label}) {
for (final conditionValue in [
...conditions.values,
if (defaultValue != null) defaultValue
Expand Down Expand Up @@ -81,6 +86,7 @@ Logic cases(Logic expression, Map<dynamic, dynamic> conditions,
}

final result = Logic(name: 'result', width: width, naming: Naming.mergeable);
var labelNum = 0;

Combinational([
Case(
Expand All @@ -91,10 +97,12 @@ Logic cases(Logic expression, Map<dynamic, dynamic> conditions,
condition.key is Logic
? condition.key as Logic
: Const(condition.key, width: expression.width),
[result < condition.value])
[result < condition.value],
label: label == null ? null : '${label}_case${labelNum++}')
],
conditionalType: conditionalType,
defaultItem: defaultValue != null ? [result < defaultValue] : null)
defaultItem: defaultValue != null ? [result < defaultValue] : null,
defaultLabel: label == null ? null : '${label}_default')
]);

return result;
Expand All @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


/// The type of case block this is, for special attributes
/// (e.g. [ConditionalType.unique], [ConditionalType.priority]).
///
Expand All @@ -130,7 +141,8 @@ class Case extends Conditional {
/// If none of [items] match, then [defaultItem] is executed.
Case(this.expression, this.items,
{List<Conditional>? defaultItem,
this.conditionalType = ConditionalType.none})
this.conditionalType = ConditionalType.none,
this.defaultLabel})
: _defaultItem = defaultItem {
for (final item in items) {
if (item.value.width != expression.width) {
Expand Down Expand Up @@ -267,25 +279,30 @@ class Case extends Conditional {
final subPadding = Conditional.calcPadding(indent + 2);
for (final item in items) {
final conditionName = inputsNameMap[driverInput(item.value).name];
final caseLabel =
item.label == null ? '' : ' : ${Sanitizer.sanitizeSV(item.label!)}';
final caseContents = item.then
.map((conditional) => conditional.verilogContents(
indent + 4, inputsNameMap, outputsNameMap, assignOperator))
.join('\n');
verilog.write('''
$subPadding$conditionName : begin
$subPadding$conditionName : begin$caseLabel
$caseContents
${subPadding}end
${subPadding}end$caseLabel
''');
}
if (defaultItem != null) {
final defaultCaseContents = defaultItem!
.map((conditional) => conditional.verilogContents(
indent + 4, inputsNameMap, outputsNameMap, assignOperator))
.join('\n');
final defaultCaseLabel = defaultLabel == null
? ''
: ' : ${Sanitizer.sanitizeSV(defaultLabel!)}';
verilog.write('''
${subPadding}default : begin
${subPadding}default : begin$defaultCaseLabel
$defaultCaseContents
${subPadding}end
${subPadding}end$defaultCaseLabel
''');
}
verilog.write('${padding}endcase\n');
Expand Down Expand Up @@ -379,7 +396,7 @@ class CaseZ extends Case {
///
/// If none of [items] match, then [defaultItem] is executed.
CaseZ(super.expression, super.items,
{super.defaultItem, super.conditionalType});
{super.defaultItem, super.conditionalType, super.defaultLabel});

@override
String get caseType => 'casez';
Expand Down
8 changes: 5 additions & 3 deletions lib/src/modules/conditionals/combinational.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ class Combinational extends Always {
/// If any "write after read" occurs, then a [WriteAfterReadException] will
/// be thrown since it could lead to a mismatch between simulation and
/// synthesis. See [Combinational.ssa] for more details.
Combinational(super._conditionals, {super.name = 'combinational'}) {
Combinational(super._conditionals,
{super.name = 'combinational', super.label}) {
_execute(); // for initial values
for (final driver in assignedDriverToInputMap.keys) {
driver.glitch.listen((args) {
Expand Down Expand Up @@ -89,7 +90,8 @@ class Combinational extends Always {
/// that it will not be noticeable.
factory Combinational.ssa(
List<Conditional> Function(Logic Function(Logic signal) s) construct,
{String name = 'combinational_ssa'}) {
{String name = 'combinational_ssa',
String? label}) {
final context = _ssaContextCounter++;

final ssas = <SsaLogic>[];
Expand All @@ -109,7 +111,7 @@ class Combinational extends Always {
// no need to keep any of this old info around anymore
signalToSsaDrivers.clear();

return Combinational(conditionals, name: name);
return Combinational(conditionals, name: name, label: label);
}

/// A map from [SsaLogic]s to signals that they drive.
Expand Down
11 changes: 10 additions & 1 deletion lib/src/modules/conditionals/flop.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ Logic flop(
Logic? reset,
dynamic resetValue,
bool asyncReset = false,
String? label,
}) =>
FlipFlop(
clk,
Expand All @@ -39,6 +40,7 @@ Logic flop(
reset: reset,
resetValue: resetValue,
asyncReset: asyncReset,
label: label,
).q;

/// Represents a single flip-flop with no reset.
Expand Down Expand Up @@ -92,6 +94,9 @@ class FlipFlop extends Module with SystemVerilog {
/// reset. If no `reset` is provided, this will have no effect.
final bool asyncReset;

/// An optional label for the blocks in this flop.
final String? label;

/// Constructs a flip flop which is positive edge triggered on [clk].
///
/// When optional [en] is provided, an additional input will be created for
Expand All @@ -115,6 +120,7 @@ class FlipFlop extends Module with SystemVerilog {
dynamic resetValue,
this.asyncReset = false,
super.name = 'flipflop',
this.label,
}) {
if (clk.width != 1) {
throw Exception('clk must be 1 bit');
Expand Down Expand Up @@ -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')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

];
}

Sequential(
Expand All @@ -156,6 +164,7 @@ class FlipFlop extends Module with SystemVerilog {
asyncReset: asyncReset,
resetValues:
_reset != null ? {q: _resetValuePort ?? _resetValueConst} : null,
label: label,
);
}

Expand Down
30 changes: 21 additions & 9 deletions lib/src/modules/conditionals/if.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import 'package:collection/collection.dart';
import 'package:meta/meta.dart';
import 'package:rohd/rohd.dart';
import 'package:rohd/src/modules/conditionals/ssa.dart';
import 'package:rohd/src/utilities/sanitizer.dart';

/// A conditional block to execute only if [condition] is satisified.
///
Expand All @@ -22,8 +23,11 @@ class ElseIf {
/// The [Conditional]s to execute if [condition] is satisfied.
final List<Conditional> then;

/// An optional block label.
final String? label;

/// If [condition] is 1, then [then] will be executed.
ElseIf(this.condition, this.then) {
ElseIf(this.condition, this.then, {this.label}) {
if (condition.width != 1) {
throw PortWidthMismatchException(condition, 1);
}
Expand All @@ -32,7 +36,8 @@ class ElseIf {
/// If [condition] is 1, then [then] will be executed.
///
/// Use this constructor when you only have a single [then] condition.
ElseIf.s(Logic condition, Conditional then) : this(condition, [then]);
ElseIf.s(Logic condition, Conditional then, {String? label})
: this(condition, [then], label: label);
}

/// A conditional block to execute only if `condition` is satisified.
Expand All @@ -46,13 +51,14 @@ typedef Iff = ElseIf;
class Else extends Iff {
/// If none of the proceding [Iff] or [ElseIf] are executed, then
/// [then] will be executed.
Else(List<Conditional> then) : super(Const(1), then);
Else(List<Conditional> then, {String? label})
: super(Const(1), then, label: label);

/// If none of the proceding [Iff] or [ElseIf] are executed, then
/// [then] will be executed.
///
/// Use this constructor when you only have a single [then] condition.
Else.s(Conditional then) : this([then]);
Else.s(Conditional then, {String? label}) : this([then], label: label);
}

/// Represents a chain of blocks of code to be conditionally executed, like
Expand Down Expand Up @@ -82,10 +88,14 @@ class If extends Conditional {

/// If [condition] is high, then [then] executes, otherwise [orElse] is
/// executed.
If(Logic condition, {List<Conditional>? then, List<Conditional>? orElse})
If(Logic condition,
{List<Conditional>? then,
List<Conditional>? orElse,
String? ifLabel,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd agree with this, I don't think most people are labelling their if/else blocks too much. I've seen it for critical debug paths in synthesis but a simple name is probably sufficient.

String? elseLabel})
: this.block([
Iff(condition, then ?? []),
if (orElse != null) Else(orElse),
Iff(condition, then ?? [], label: ifLabel),
if (orElse != null) Else(orElse, label: elseLabel),
]);

/// If [condition] is high, then [then] is excutes,
Expand Down Expand Up @@ -198,10 +208,12 @@ class If extends Conditional {
indent + 2, inputsNameMap, outputsNameMap, assignOperator))
.join('\n');
final condition = iff is! Else ? '($conditionName)' : '';
final iffLabel =
iff.label == null ? '' : ' : ${Sanitizer.sanitizeSV(iff.label!)}';
verilog.write('''
$padding$header$condition begin
$padding$header$condition begin$iffLabel
$ifContents
${padding}end ''');
${padding}end$iffLabel ''');
}
verilog.write('\n');

Expand Down
3 changes: 3 additions & 0 deletions lib/src/modules/conditionals/sequential.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ class Sequential extends Always {
bool asyncReset = false,
bool allowMultipleAssignments = true,
String name = 'sequential',
String? label,
}) : this.multi(
[clk],
conditionals,
Expand All @@ -173,6 +174,7 @@ class Sequential extends Always {
asyncReset: asyncReset,
resetValues: resetValues,
allowMultipleAssignments: allowMultipleAssignments,
label: label,
);

/// Constructs a [Sequential] multi-triggered by any of [posedgeTriggers] and
Expand Down Expand Up @@ -210,6 +212,7 @@ class Sequential extends Always {
super.name = 'sequential',
this.allowMultipleAssignments = true,
List<Logic> negedgeTriggers = const [],
super.label,
}) : super(reset: reset) {
_registerInputTriggers([
...posedgeTriggers,
Expand Down
Loading
Loading