Repository: intel/rohd
Pull request #447 (2023-12-26 23:26:08)

Pipeline fixes and improvements
<!-- Please make sure you check out the contribution guidelines before submitting a pull request! -->

## Description & Motivation

The `Pipeline` abstraction has some bugs and untested behavior, so this PR addresses those:
- Add a new exception for when a port does not exist, and throw it if `get` is called on a pipeline for an un-piped signal.
- Mark some SSA signals as `mergeable` to make generated SystemVerilog prettier and less verbose.
- Expose `stageCount` for `Pipeline`
- Improve documentation comments for pipelining.
- Fix a bug where `get` on pipe stages which did not modify a signal returns a floating signal.
- Add a bunch of new tests for pipelining.

## Related Issue(s)

N/A

## Testing

Existing tests + new tests added

## Backwards-compatibility

> Is this a breaking change that will not be backwards-compatible? If yes, how so?

No

## Documentation

> Does the change require any updates to documentation? If so, where? Are they included?

No



Patch:
diff --git a/lib/src/exceptions/module/module_exceptions.dart b/lib/src/exceptions/module/module_exceptions.dart
index 115eab91..0c6038c2 100644
--- a/lib/src/exceptions/module/module_exceptions.dart
+++ b/lib/src/exceptions/module/module_exceptions.dart
@@ -2,4 +2,5 @@
 // SPDX-License-Identifier: BSD-3-Clause
 
 export 'module_not_built_exception.dart';
+export 'port_does_not_exist_exception.dart';
 export 'port_width_mismatch_exception.dart';
diff --git a/lib/src/exceptions/module/port_does_not_exist_exception.dart b/lib/src/exceptions/module/port_does_not_exist_exception.dart
new file mode 100644
index 00000000..e626cd79
--- /dev/null
+++ b/lib/src/exceptions/module/port_does_not_exist_exception.dart
@@ -0,0 +1,16 @@
+// Copyright (C) 2023 Intel Corporation
+// SPDX-License-Identifier: BSD-3-Clause
+//
+// port_does_not_exist_exception.dart
+// Definition for exception when a port is not present.
+//
+// 2023 December 26
+// Author: Max Korbel <max.korbel@intel.com>
+
+import 'package:rohd/rohd.dart';
+
+/// An [Exception] thrown when a port has the wrong width.
+class PortDoesNotExistException extends RohdException {
+  /// Constructs a new [Exception] for when a port is not present.
+  PortDoesNotExistException(super.message);
+}
diff --git a/lib/src/module.dart b/lib/src/module.dart
index f8f48040..6f6403b4 100644
--- a/lib/src/module.dart
+++ b/lib/src/module.dart
@@ -94,7 +94,7 @@ abstract class Module {
   @protected
   Logic input(String name) => _inputs.containsKey(name)
       ? _inputs[name]!
-      : throw Exception(
+      : throw PortDoesNotExistException(
           'Input name "$name" not found as an input to this Module.');
 
   /// Provides the [input] named [name] if it exists, otherwise `null`.
@@ -110,7 +110,7 @@ abstract class Module {
   /// to consume this within this [Module] as well.
   Logic output(String name) => _outputs.containsKey(name)
       ? _outputs[name]!
-      : throw Exception(
+      : throw PortDoesNotExistException(
           'Output name "$name" not found as an output of this Module.');
 
   /// Provides the [output] named [name] if it exists, otherwise `null`.
diff --git a/lib/src/modules/conditional.dart b/lib/src/modules/conditional.dart
index 571bb9ca..2a576358 100644
--- a/lib/src/modules/conditional.dart
+++ b/lib/src/modules/conditional.dart
@@ -184,7 +184,7 @@ class _SsaLogic extends Logic {
 
   /// Constructs a new SSA node referring to a signal in a specific context.
   _SsaLogic(this._ref, this._context)
-      : super(width: _ref.width, name: _ref.name);
+      : super(width: _ref.width, name: _ref.name, naming: Naming.mergeable);
 }
 
 /// Represents a block of combinational logic.
@@ -1036,7 +1036,7 @@ Logic cases(Logic expression, Map<dynamic, dynamic> conditions,
     }
   }
 
-  final result = Logic(name: 'result', width: width);
+  final result = Logic(name: 'result', width: width, naming: Naming.mergeable);
 
   Combinational([
     Case(
@@ -1280,6 +1280,7 @@ ${subPadding}end
           phiMappings[localMapping.key] = Logic(
             name: '${localMapping.key.name}_phi',
             width: localMapping.key.width,
+            naming: Naming.mergeable,
           );
         }
 
@@ -1570,6 +1571,7 @@ ${padding}end ''');
           phiMappings[localMapping.key] = Logic(
             name: '${localMapping.key.name}_phi',
             width: localMapping.key.width,
+            naming: Naming.mergeable,
           );
         }
 
diff --git a/lib/src/modules/pipeline.dart b/lib/src/modules/pipeline.dart
index 8702487b..06231cca 100644
--- a/lib/src/modules/pipeline.dart
+++ b/lib/src/modules/pipeline.dart
@@ -98,11 +98,20 @@ class Pipeline {
   late final List<_PipeStage> _stages;
 
   /// Returns the number of stages in this pipeline.
-  int get _numStages => _stages.length;
+  ///
+  /// Note that this will be one greater than the number of elements in `stages`
+  /// during construction, as well as one greater than the number of flop
+  /// stages. This represents the count of chunks of combinational logic
+  /// separated by flops.
+  int get stageCount => _stages.length;
 
   /// A map of reset values for every signal.
   late final Map<Logic, dynamic> _resetValues;
 
+  /// Tracks whether this [Pipeline] is done being constructed to conditionally
+  /// run safety checks on API calls.
+  bool _constructionComplete = false;
+
   /// Constructs a simple pipeline, separating arbitrary combinational logic by
   /// flop stages.
   ///
@@ -113,6 +122,12 @@ class Pipeline {
   /// signals for a given pipe stage.  Flops are positive edge triggered
   /// based on [clk].
   ///
+  /// Then `i`th element of [stages] defines the combinational logic driving a
+  /// flop of index `i`. That is, the first entry in [stages] drives the
+  /// first set of flops, so logic defined in the first stage combinationally
+  /// consumes inputs to the [Pipeline]. The output of the [Pipeline] is
+  /// driven by flops driven by the last entry of [stages].
+  ///
   /// Signals to be pipelined can optionally be specified in the [signals]
   /// list.  Any signal referenced in a stage via the [PipelineStageInfo]
   /// will automatically be included in the entire pipeline.
@@ -124,7 +139,7 @@ class Pipeline {
   /// in [resetValues] should be a type acceptable to [Logic]'s `put` function.
   ///
   /// Each stage can be stalled independently using [stalls], where every index
-  ///  of [stalls] corresponds to the index of the stage to be stalled.  When
+  /// of [stalls] corresponds to the index of the stage to be stalled.  When
   /// a stage's stall is asserted, the output of that stage will not change.
   Pipeline(Logic clk,
       {List<List<Conditional> Function(PipelineStageInfo p)> stages = const [],
@@ -149,31 +164,30 @@ class Pipeline {
     _stages = stages.map(_PipeStage.new).toList();
     _stages.add(_PipeStage((p) => [])); // output stage
 
-    if (_numStages == 0) {
-      return;
-    }
-
     _resetValues = Map.from(resetValues);
 
     _setStalls(stalls);
 
     signals.forEach(_add);
 
-    for (var stageIndex = 0; stageIndex < _numStages; stageIndex++) {
+    for (var stageIndex = 0; stageIndex < stageCount; stageIndex++) {
       Combinational.ssa((ssa) {
         // keep track of the previously registered logics:
         final prevRegisteredLogics = _registeredLogics.toSet();
 
         // build the conditionals first so that we populate _registeredLogics
-        final stageConditionals = _stages[stageIndex]
-            .operation(PipelineStageInfo._(this, stageIndex, ssa));
+        final stageConditionals = _stages[stageIndex].operation(
+          PipelineStageInfo._(this, stageIndex, ssa),
+        );
 
         // if any new logics were registered, add some extra assignments
         // to make up the gap since it didn't get included in prior generations
         for (final l in _registeredLogics) {
           if (!prevRegisteredLogics.contains(l)) {
             for (var i = 0; i < stageIndex; i++) {
-              _o(l, i) <= _i(l, i);
+              // make sure to hook up in-to-out through main for .get's
+              get(l, i) <= _i(l, i);
+              _o(l, i) <= get(l, i);
             }
           }
         }
@@ -190,16 +204,18 @@ class Pipeline {
         _o(l, stageIndex) <= get(l, stageIndex);
       }
     }
+
+    _constructionComplete = true;
   }
 
   /// Sets up the stall signals across [_stages].
   void _setStalls(List<Logic?>? stalls) {
     if (stalls != null) {
-      if (stalls.length != _numStages - 1) {
+      if (stalls.length != stageCount - 1) {
         throw Exception('Stall list length (${stalls.length}) must match '
-            'number of stages (${_numStages - 1}).');
+            'number of stages (${stageCount - 1}).');
       }
-      for (var i = 0; i < _numStages - 1; i++) {
+      for (var i = 0; i < stageCount - 1; i++) {
         final stall = stalls[i];
         if (stall == null) {
           continue;
@@ -230,7 +246,7 @@ class Pipeline {
     }
 
     var ffAssignsWithStall =
-        List<Conditional>.generate(_numStages - 1, (index) {
+        List<Conditional>.generate(stageCount - 1, (index) {
       final stall = _stages[index].stall;
       final ffAssign = ffAssigns[index] as ConditionalAssign;
       final driver = stall != null
@@ -283,12 +299,20 @@ class Pipeline {
   /// Gets the pipelined version of [logic].  By default [stageIndex] is the
   /// last stage (the output of the pipeline).
   ///
-  /// If the signal is not already a part of this [Pipeline], the signal will be
-  /// added to the [Pipeline].  Use [stageIndex] to select the value of [logic]
-  /// at a specific stage of the pipeline.
+  /// During construction, if the signal is not already a part of this
+  /// [Pipeline], the signal will be added to the [Pipeline]. After
+  /// construction, only signals registered during construction can be accessed.
+  ///
+  /// Use [stageIndex] to select the value of [logic] at a specific stage of the
+  /// pipeline. The [stageIndex] must be less than [stageCount].
   Logic get(Logic logic, [int? stageIndex]) {
     if (!_isRegistered(logic)) {
-      _add(logic);
+      if (_constructionComplete) {
+        throw PortDoesNotExistException(
+            'Signal $logic was not piped through this Pipeline.');
+      } else {
+        _add(logic);
+      }
     }
 
     stageIndex ??= _stages.length - 1;


Test patch:
diff --git a/test/comb_math_test.dart b/test/comb_math_test.dart
index c58881db..b2a7165e 100644
--- a/test/comb_math_test.dart
+++ b/test/comb_math_test.dart
@@ -83,7 +83,7 @@ class SimplerExampleSsa extends Module {
     a = addInput('a', a, width: 8);
     addOutput('b', width: 8);
 
-    final inner = Logic(name: 'inner', width: 8);
+    final inner = Logic(name: 'inner', width: 8, naming: Naming.mergeable);
 
     Combinational.ssa((s) => [
           s(inner) < 0xf,
diff --git a/test/comb_mod_test.dart b/test/comb_mod_test.dart
index 45b3e82d..bd09ceb9 100644
--- a/test/comb_mod_test.dart
+++ b/test/comb_mod_test.dart
@@ -13,7 +13,7 @@ import 'package:test/test.dart';
 
 class IncrModule extends Module {
   Logic get result => output('result');
-  IncrModule(Logic toIncr) {
+  IncrModule(Logic toIncr) : super(name: 'incr') {
     toIncr = addInput('toIncr', toIncr, width: toIncr.width);
     addOutput('result', width: toIncr.width);
     result <= toIncr + 1;
diff --git a/test/pipeline_test.dart b/test/pipeline_test.dart
index 96769660..f00e28bf 100644
--- a/test/pipeline_test.dart
+++ b/test/pipeline_test.dart
@@ -26,6 +26,25 @@ class SimplePipelineModule extends Module {
   }
 }
 
+class SimplePipelineModuleStages extends Module {
+  SimplePipelineModuleStages(Logic a) : super(name: 'simple_pipeline_module') {
+    final clk = SimpleClockGenerator(10).clk;
+    a = addInput('a', a, width: a.width);
+    final b = addOutput('b', width: a.width);
+
+    final pipeline = Pipeline(clk, stages: [
+      (p) => [p.get(a) < p.get(a) + 1],
+      (p) => [p.get(a) < p.get(a) + 1],
+      (p) => [p.get(a) < p.get(a) + 1],
+    ]);
+    b <= pipeline.get(a);
+
+    for (var i = 0; i < pipeline.stageCount; i++) {
+      addOutput('a$i', width: a.width) <= pipeline.get(a, i);
+    }
+  }
+}
+
 class SimplePipelineModuleLateAdd extends Module {
   SimplePipelineModuleLateAdd(Logic a)
       : super(name: 'simple_pipeline_module_late_add') {
@@ -45,6 +64,73 @@ class SimplePipelineModuleLateAdd extends Module {
   }
 }
 
+class EmptyPipe extends Module {
+  Logic get b => output('b');
+  EmptyPipe(Logic a) : super(name: 'pipeline_uninit') {
+    final clk = SimpleClockGenerator(10).clk;
+    a = addInput('a', a, width: a.width);
+    final b = addOutput('b', width: a.width);
+
+    final pipeline = Pipeline(clk, signals: [
+      a,
+    ], stages: [
+      for (var i = 0; i < 5; i++) (p) => [],
+    ]);
+
+    b <= pipeline.get(a);
+  }
+}
+
+class PipelineInitDirect extends Module {
+  Logic get b => output('b');
+  PipelineInitDirect(Logic a) : super(name: 'pipeline_uninit') {
+    final clk = SimpleClockGenerator(10).clk;
+    a = addInput('a', a, width: a.width);
+    final b = addOutput('b', width: a.width);
+
+    final c = Logic(width: a.width, name: 'c');
+
+    final pipeline = Pipeline(clk, stages: [
+      (p) => [c < a],
+      (p) => [],
+      (p) => [],
+      (p) => [p.get(c) < p.get(c) + 1],
+      (p) => [p.get(c) < p.get(c) + 1],
+      (p) => [p.get(c) < p.get(c) + 1],
+    ]);
+
+    b <=
+        List.generate(pipeline.stageCount, (index) => pipeline.get(c, index))
+            .reduce((a, b) => a | b);
+  }
+}
+
+class PipelineInitWithGet extends Module {
+  Logic get b => output('b');
+  PipelineInitWithGet(Logic a) : super(name: 'pipeline_uninit') {
+    final clk = SimpleClockGenerator(10).clk;
+    a = addInput('a', a, width: a.width);
+    final b = addOutput('b', width: a.width);
+
+    final c = Logic(width: a.width, name: 'c');
+
+    final pipeline = Pipeline(clk, stages: [
+      (p) => [
+            p.get(c) < a,
+          ],
+      (p) => [],
+      (p) => [],
+      (p) => [p.get(c) < p.get(c) + 1],
+      (p) => [p.get(c) < p.get(c) + 1],
+      (p) => [p.get(c) < p.get(c) + 1],
+    ]);
+
+    b <=
+        List.generate(pipeline.stageCount, (index) => pipeline.get(c, index))
+            .reduce((a, b) => a | b);
+  }
+}
+
 class RVPipelineModule extends Module {
   RVPipelineModule(Logic a, Logic reset, Logic validIn, Logic readyForOut)
       : super(name: 'rv_pipeline_module') {
@@ -129,6 +215,64 @@ void main() {
       SimCompare.checkIverilogVector(pipem, vectors);
     });
 
+    test('simple pipeline with intermediate gets', () async {
+      final pipem = SimplePipelineModuleStages(Logic(width: 8));
+      await pipem.build();
+
+      final vectors = [
+        Vector({'a': 1}, {'a0': 2}),
+        Vector({'a': 2}, {'a0': 3, 'a1': 3}),
+        Vector({'a': 3}, {'a0': 4, 'a1': 4, 'a2': 4}),
+        Vector({'a': 4}, {'a0': 5, 'a1': 5, 'a2': 5, 'b': 4}),
+        Vector({'a': 5}, {'a0': 6, 'a1': 6, 'a2': 6, 'a3': 5, 'b': 5}),
+        Vector({'a': 6}, {'b': 6}),
+      ];
+      await SimCompare.checkFunctionalVector(pipem, vectors);
+      SimCompare.checkIverilogVector(pipem, vectors);
+    });
+
+    test('getting out of range on pipeline is error', () {
+      final x = Logic();
+      expect(
+          () => Pipeline(Logic(), signals: [
+                x
+              ], stages: [
+                (p) => [],
+                (p) => [],
+              ]).get(x, 3),
+          throwsRangeError);
+    });
+
+    test('getting unregisterd signal on pipeline is error', () {
+      expect(
+          () => Pipeline(Logic(), signals: [
+                Logic()
+              ], stages: [
+                (p) => [],
+                (p) => [],
+              ]).get(Logic()),
+          throwsA(isA<PortDoesNotExistException>()));
+    });
+
+    test('empty pipeline', () async {
+      final pipem = EmptyPipe(Logic(width: 8));
+      await pipem.build();
+
+      final vectors = [
+        Vector({'a': 1}, {}),
+        Vector({'a': 2}, {}),
+        Vector({'a': 3}, {}),
+        Vector({'a': 4}, {}),
+        Vector({'a': 5}, {}),
+        Vector({'a': 6}, {'b': 1}),
+        Vector({'a': 7}, {'b': 2}),
+        Vector({'a': 8}, {'b': 3}),
+        Vector({'a': 9}, {'b': 4}),
+      ];
+      await SimCompare.checkFunctionalVector(pipem, vectors);
+      SimCompare.checkIverilogVector(pipem, vectors);
+    });
+
     test('multiuse pipeline', () async {
       final pipem =
           PipelineWithMultiUseModule(Logic(width: 8), Logic(width: 8));
@@ -164,6 +308,44 @@ void main() {
       SimCompare.checkIverilogVector(pipem, vectors);
     });
 
+    test('pipeline initialized via get', () async {
+      final pipem = PipelineInitWithGet(Logic(width: 8));
+      await pipem.build();
+
+      final vectors = [
+        Vector({'a': 1}, {}),
+        Vector({'a': 2}, {}),
+        Vector({'a': 3}, {}),
+        Vector({'a': 4}, {}),
+        Vector({'a': 4}, {}),
+        Vector({'a': 4}, {}),
+      ];
+      await SimCompare.checkFunctionalVector(pipem, vectors);
+
+      expect(pipem.b.value.isValid, isTrue);
+
+      SimCompare.checkIverilogVector(pipem, vectors);
+    });
+
+    test('pipeline initialized directly instead of via get', () async {
+      final pipem = PipelineInitDirect(Logic(width: 8));
+      await pipem.build();
+
+      final vectors = [
+        Vector({'a': 1}, {}),
+        Vector({'a': 2}, {}),
+        Vector({'a': 3}, {}),
+        Vector({'a': 4}, {}),
+        Vector({'a': 4}, {}),
+        Vector({'a': 4}, {}),
+      ];
+      await SimCompare.checkFunctionalVector(pipem, vectors);
+
+      expect(pipem.b.value.isValid, isTrue);
+
+      SimCompare.checkIverilogVector(pipem, vectors);
+    });
+
     test('rv pipeline simple', () async {
       final pipem =
           RVPipelineModule(Logic(width: 8), Logic(), Logic(), Logic());