Repository: intel/rohd (dart)

Issue:
Sort ports in generated outputs
### Motivation

Currently the order of ports in generated SystemVerilog is somewhat random, which can make integration of the generated code a little annoying.  It would be nice if the ports were organized.

### Desired solution

It might be reasonable to sort them by name alphabetically, perhaps grouped only by name or maybe keeping inputs above outputs.  Usually related ports might have the same prefix.

### Alternatives considered

_No response_

### Additional details

_No response_

Pull request description:
<!-- Please make sure you check out the contribution guidelines before submitting a pull request! -->

## Description & Motivation

Sort ports and internal signal declarations alphabetically in generated SystemVerilog.

## Related Issue(s)

Fix #395

## Testing

Added new test that checks it is sorted (confirmed failure without fix).

## Backwards-compatibility

> Is this a breaking change that will not be backwards-compatible? If yes, how so?

SV generated will change, but will still be functionally equivalent.

## Documentation

> Does the change require any updates to documentation? If so, where? Are they included?

No

Patch:
diff --git a/lib/src/synthesizers/systemverilog.dart b/lib/src/synthesizers/systemverilog.dart
index 68c182aed..b0598f15a 100644
--- a/lib/src/synthesizers/systemverilog.dart
+++ b/lib/src/synthesizers/systemverilog.dart
@@ -7,6 +7,7 @@
 // 2021 August 26
 // Author: Max Korbel <max.korbel@intel.com>
 
+import 'package:collection/collection.dart';
 import 'package:rohd/rohd.dart';
 import 'package:rohd/src/collections/traverseable_collection.dart';
 import 'package:rohd/src/utilities/uniquifier.dart';
@@ -50,11 +51,11 @@ class SystemVerilogSynthesizer extends Synthesizer {
     final connections = <String>[];
 
     // ignore: invalid_use_of_protected_member
-    module.inputs.forEach((signalName, logic) {
+    module.inputs.keys.sorted((a, b) => a.compareTo(b)).forEach((signalName) {
       connections.add('.$signalName(${inputs[signalName]})');
     });
 
-    module.outputs.forEach((signalName, logic) {
+    module.outputs.keys.sorted((a, b) => a.compareTo(b)).forEach((signalName) {
       connections.add('.$signalName(${outputs[signalName]})');
     });
 
@@ -152,6 +153,7 @@ class _SystemVerilogSynthesisResult extends SynthesisResult {
 
   List<String> _verilogInputs() {
     final declarations = _synthModuleDefinition.inputs
+        .sorted((a, b) => a.name.compareTo(b.name))
         .map((sig) => 'input logic ${sig.definitionName()}')
         .toList(growable: false);
     return declarations;
@@ -159,6 +161,7 @@ class _SystemVerilogSynthesisResult extends SynthesisResult {
 
   List<String> _verilogOutputs() {
     final declarations = _synthModuleDefinition.outputs
+        .sorted((a, b) => a.name.compareTo(b.name))
         .map((sig) => 'output logic ${sig.definitionName()}')
         .toList(growable: false);
     return declarations;
@@ -166,7 +169,8 @@ class _SystemVerilogSynthesisResult extends SynthesisResult {
 
   String _verilogInternalNets() {
     final declarations = <String>[];
-    for (final sig in _synthModuleDefinition.internalNets) {
+    for (final sig in _synthModuleDefinition.internalNets
+        .sorted((a, b) => a.name.compareTo(b.name))) {
       if (sig.needsDeclaration) {
         declarations.add('logic ${sig.definitionName()};');
       }


Test patch:
diff --git a/test/sv_gen_test.dart b/test/sv_gen_test.dart
new file mode 100644
index 000000000..063436a7d
--- /dev/null
+++ b/test/sv_gen_test.dart
@@ -0,0 +1,120 @@
+// Copyright (C) 2023 Intel Corporation
+// SPDX-License-Identifier: BSD-3-Clause
+//
+// sv_gen_test.dart
+// Tests for SystemVerilog generation.
+//
+// 2023 October 4
+// Author: Max Korbel <max.korbel@intel.com>
+
+import 'package:collection/collection.dart';
+import 'package:rohd/rohd.dart';
+import 'package:test/test.dart';
+
+class AlphabeticalModule extends Module {
+  AlphabeticalModule(Logic l, Logic a, Logic w) {
+    l = addInput('l', l);
+    a = addInput('a', a);
+    w = addInput('w', w);
+
+    final o = Logic(name: 'o');
+    final c = Logic(name: 'c');
+    final y = Logic(name: 'y');
+
+    c <= l & w;
+    o <= a | l;
+    y <= w ^ l;
+
+    addOutput('m');
+    addOutput('x') <= c + o + y;
+    addOutput('b');
+  }
+}
+
+class AlphabeticalWidthsModule extends Module {
+  AlphabeticalWidthsModule() {
+    final l = addInput('l', Logic(width: 4), width: 4);
+    final a = addInput('a', Logic(width: 3), width: 3);
+    final w = addInput('w', Logic(width: 2), width: 2);
+
+    final o = Logic(name: 'o', width: 4);
+    final c = Logic(name: 'c', width: 3);
+    final y = Logic(name: 'y', width: 2);
+
+    c <= a & a;
+    o <= l | l;
+    y <= w ^ w;
+
+    addOutput('m', width: 4) <= o + o;
+    addOutput('x', width: 2) <= y + y;
+    addOutput('b', width: 3) <= c + c;
+  }
+}
+
+class AlphabeticalSubmodulePorts extends Module {
+  AlphabeticalSubmodulePorts() {
+    final l = addInput('l', Logic());
+    final a = addInput('a', Logic());
+    final w = addInput('w', Logic());
+
+    final am = AlphabeticalModule(l, a, w);
+
+    addOutput('m') <= am.output('m');
+    addOutput('x') <= am.output('x');
+    addOutput('b') <= am.output('b');
+  }
+}
+
+void main() {
+  group('signal declaration order', () {
+    void checkSignalDeclarationOrder(String sv, List<String> signalNames) {
+      final expected =
+          signalNames.map((e) => RegExp(r'logic\s*\[?[:\d\s]*]?\s*' + e));
+      final indices = expected.map(sv.indexOf);
+      expect(indices.isSorted((a, b) => a.compareTo(b)), isTrue,
+          reason: 'Expected order $signalNames, but indices were $indices');
+    }
+
+    test('input, output, and internal signals are sorted', () async {
+      final mod = AlphabeticalModule(Logic(), Logic(), Logic());
+      await mod.build();
+      final sv = mod.generateSynth();
+
+      checkSignalDeclarationOrder(sv, ['a', 'l', 'w']);
+      checkSignalDeclarationOrder(sv, ['b', 'm', 'x']);
+      checkSignalDeclarationOrder(sv, ['c', 'o', 'y']);
+
+      checkSignalDeclarationOrder(
+          sv, ['a', 'l', 'w', 'b', 'm', 'x', 'c', 'o', 'y']);
+    });
+
+    test('input, output, and internal signals are sorted (different widths)',
+        () async {
+      final mod = AlphabeticalWidthsModule();
+      await mod.build();
+      final sv = mod.generateSynth();
+
+      checkSignalDeclarationOrder(sv, ['a', 'l', 'w']);
+      checkSignalDeclarationOrder(sv, ['b', 'm', 'x']);
+      checkSignalDeclarationOrder(sv, ['c', 'o', 'y']);
+
+      checkSignalDeclarationOrder(
+          sv, ['a', 'l', 'w', 'b', 'm', 'x', 'c', 'o', 'y']);
+    });
+  });
+
+  test('submodule port connections input, output are sorted', () async {
+    void checkPortConnectionOrder(String sv, List<String> signalNames) {
+      final expected = signalNames.map((e) => '.$e($e)');
+      final indices = expected.map(sv.indexOf);
+      expect(indices.isSorted((a, b) => a.compareTo(b)), isTrue,
+          reason: 'Expected order $signalNames, but indices were $indices');
+    }
+
+    final mod = AlphabeticalSubmodulePorts();
+    await mod.build();
+    final sv = mod.generateSynth();
+
+    checkPortConnectionOrder(sv, ['a', 'l', 'w', 'b', 'm', 'x']);
+  });
+}