Repository: intel/rohd (dart)

Issue:
Make a shortcut to create a 0-width `LogicValue`
### Motivation

There are multiple ways to create a `LogicValue` with no width, but none of them feel that clean.

Something like `LogicValue.empty` which always returns a 0-width `LogicValue`, ideally the most efficiently implemented way, would be convenient.

### Desired solution

Add a new constant to `LogicValue` to create a zero-width value.

### Alternatives considered

Just use `LogicValue.of([])`, or something similar.

### Additional details

_No response_

Pull request:
<!-- Please make sure you check out the contribution guidelines before submitting a pull request! -->

## Description & Motivation

String manipulation is not ideal for performance but was used in a number of places for `LogicValue` operations.  This PR removes some of those places where possible.

See conversation that initiated some of these changes here: https://github.com/intel/rohd/pull/213#discussion_r1037443152

This PR specifically fixes:
- Conversion to unsigned `int` from `BigInt` to not use `String`
- `LogicValue.of` to not use `String` (which also optimizes all of the `swizzle` operations on `LogicValue`)

## Related Issue(s)

Fixed #202 (since it was convenient for implementation)

Improved upon fixes in #213

## Testing

Added new tests to cover potential corner cases

## 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/benchmark/benchmark.dart b/benchmark/benchmark.dart
index add1d3ed6..5066fa561 100644
--- a/benchmark/benchmark.dart
+++ b/benchmark/benchmark.dart
@@ -8,8 +8,12 @@
 /// Author: Max Korbel <max.korbel@intel.com>
 ///
 
+import 'byte_enable_benchmark.dart';
+import 'logic_value_of_benchmark.dart';
 import 'pipeline_benchmark.dart';
 
 void main() async {
   await PipelineBenchmark().report();
+  LogicValueOfBenchmark().report();
+  ByteEnableBenchmark().report();
 }
diff --git a/benchmark/benchmarks.dart b/benchmark/benchmarks.dart
deleted file mode 100644
index 13de7e7aa..000000000
--- a/benchmark/benchmarks.dart
+++ /dev/null
@@ -1,4 +0,0 @@
-/// Copyright (C) 2022 Intel Corporation
-/// SPDX-License-Identifier: BSD-3-Clause
-
-export 'pipeline_benchmark.dart';
diff --git a/benchmark/byte_enable_benchmark.dart b/benchmark/byte_enable_benchmark.dart
new file mode 100644
index 000000000..10a2c85bf
--- /dev/null
+++ b/benchmark/byte_enable_benchmark.dart
@@ -0,0 +1,54 @@
+/// Copyright (C) 2022 Intel Corporation
+/// SPDX-License-Identifier: BSD-3-Clause
+///
+/// byte_enable_benchmark.dart
+/// Benchmarking for simple byte enable hardware
+///
+/// 2022 December 2
+/// Author: Max Korbel <max.korbel@intel.com>
+///
+
+import 'package:benchmark_harness/benchmark_harness.dart';
+import 'package:rohd/rohd.dart';
+
+class ByteEnableBenchmark extends BenchmarkBase {
+  late Logic result;
+  late Logic select;
+  late Logic original;
+
+  static const int numBytes = 128;
+
+  final List<LogicValue> vectors = List.generate(
+      numBytes,
+      (index) => LogicValue.ofString(
+          (index.toRadixString(2) * numBytes).substring(0, numBytes)));
+
+  ByteEnableBenchmark() : super('ByteEnable');
+
+  @override
+  void setup() {
+    select = Logic(name: 'select', width: numBytes);
+    original = Logic(name: 'original', width: numBytes * 8);
+    original.put(LogicValue.ofString('0x1z10xz' * numBytes));
+    select.put(0);
+    result = List.generate(
+        numBytes,
+        (index) => mux(
+              select[index],
+              original.getRange(index * 8, (index + 1) * 8),
+              Const(0, width: 8),
+            )).swizzle();
+  }
+
+  @override
+  void teardown() {}
+
+  @override
+  void run() {
+    vectors.forEach(select.put);
+  }
+}
+
+void main() {
+  ByteEnableBenchmark().report();
+}
diff --git a/benchmark/logic_value_of_benchmark.dart b/benchmark/logic_value_of_benchmark.dart
new file mode 100644
index 000000000..9d7e57e14
--- /dev/null
+++ b/benchmark/logic_value_of_benchmark.dart
@@ -0,0 +1,46 @@
+/// Copyright (C) 2022 Intel Corporation
+/// SPDX-License-Identifier: BSD-3-Clause
+///
+/// logic_value_of_benchmark.dart
+/// Benchmarking for concatenation of values together into one.
+///
+/// 2022 December 1
+/// Author: Max Korbel <max.korbel@intel.com>
+///
+
+import 'dart:math';
+
+import 'package:benchmark_harness/benchmark_harness.dart';
+import 'package:rohd/rohd.dart';
+
+class LogicValueOfBenchmark extends BenchmarkBase {
+  late List<LogicValue> toOf;
+
+  LogicValueOfBenchmark() : super('LogicValueOf');
+
+  @override
+  void setup() {
+    final rand = Random(1234);
+    toOf = List.generate(
+        1000,
+        (index) => LogicValue.ofString(([
+              '0' * rand.nextInt(10),
+              '1' * rand.nextInt(10),
+              'x' * rand.nextInt(10),
+              'z' * rand.nextInt(10),
+            ]..shuffle(rand))
+                .join()));
+  }
+
+  @override
+  void teardown() {}
+
+  @override
+  void run() {
+    LogicValue.of(toOf);
+  }
+}
+
+void main() async {
+  LogicValueOfBenchmark().report();
+}
diff --git a/lib/src/modules/bus.dart b/lib/src/modules/bus.dart
index fd79b964b..8daee20da 100644
--- a/lib/src/modules/bus.dart
+++ b/lib/src/modules/bus.dart
@@ -147,8 +147,7 @@ class Swizzle extends Module with InlineSystemVerilog {
 
   /// Executes the functional behavior of this gate.
   void _execute() {
-    final updatedVal =
-        _swizzleInputs.reversed.map((e) => e.value).toList().swizzle();
+    final updatedVal = LogicValue.of(_swizzleInputs.map((e) => e.value));
     out.put(updatedVal);
   }
 
diff --git a/lib/src/values/big_logic_value.dart b/lib/src/values/big_logic_value.dart
index 2e89dc3ac..78ce405b1 100644
--- a/lib/src/values/big_logic_value.dart
+++ b/lib/src/values/big_logic_value.dart
@@ -21,8 +21,13 @@ extension BigLogicValueBigIntUtilities on BigInt {
       throw Exception('Cannot convert to BigInt when width $width'
           ' is greater than ${LogicValue._INT_BITS}');
     } else if (width == LogicValue._INT_BITS) {
-      // With `0x` in front of a hex literal, will be interpreted as unsigned.
-      return int.parse('0x${toRadixString(16)}');
+      // When width is 64, `BigInt.toInt()` will clamp values assuming that
+      // it's a signed number.  To avoid that, if the width is 64, then do the
+      // conversion in two 32-bit chunks and bitwise-or them together.
+      const maskWidth = 32;
+      final mask = _BigLogicValue._maskOfWidth(maskWidth);
+      return (this & mask).toInt() |
+          (((this >> maskWidth) & mask).toInt() << maskWidth);
     } else {
       return toInt();
     }
@@ -198,4 +203,16 @@ class _BigLogicValue extends LogicValue {
               shamt,
           _invalid >> shamt,
           width);
+
+  @override
+  BigInt get _bigIntInvalid => _invalid;
+
+  @override
+  BigInt get _bigIntValue => _value;
+
+  @override
+  int get _intInvalid => _invalid.toIntUnsigned(width);
+
+  @override
+  int get _intValue => _value.toIntUnsigned(width);
 }
diff --git a/lib/src/values/filled_logic_value.dart b/lib/src/values/filled_logic_value.dart
index 63b129ec6..894c94b1d 100644
--- a/lib/src/values/filled_logic_value.dart
+++ b/lib/src/values/filled_logic_value.dart
@@ -244,4 +244,28 @@ class _FilledLogicValue extends LogicValue {
   LogicValue _shiftArithmeticRight(int shamt) => LogicValue.ofString(
       ((_value == _LogicValueEnum.one ? '1' : '0') * shamt) +
           (this[0]._bitString() * (width - shamt)));
+
+  @override
+  BigInt get _bigIntInvalid =>
+      (_value == _LogicValueEnum.z || _value == _LogicValueEnum.x)
+          ? _BigLogicValue._maskOfWidth(width)
+          : BigInt.zero;
+
+  @override
+  BigInt get _bigIntValue =>
+      (_value == _LogicValueEnum.one || _value == _LogicValueEnum.z)
+          ? _BigLogicValue._maskOfWidth(width)
+          : BigInt.zero;
+
+  @override
+  int get _intInvalid =>
+      (_value == _LogicValueEnum.z || _value == _LogicValueEnum.x)
+          ? _SmallLogicValue._maskOfWidth(width)
+          : 0;
+
+  @override
+  int get _intValue =>
+      (_value == _LogicValueEnum.one || _value == _LogicValueEnum.z)
+          ? _SmallLogicValue._maskOfWidth(width)
+          : 0;
 }
diff --git a/lib/src/values/logic_value.dart b/lib/src/values/logic_value.dart
index 9d01be4e2..e988aa016 100644
--- a/lib/src/values/logic_value.dart
+++ b/lib/src/values/logic_value.dart
@@ -25,18 +25,21 @@ abstract class LogicValue {
   // ignore: constant_identifier_names
   static const int _INT_BITS = 64;
 
-  /// Logical value of `0`
+  /// Logical value of `0`.
   static const LogicValue zero = _FilledLogicValue(_LogicValueEnum.zero, 1);
 
-  /// Logical value of `1`
+  /// Logical value of `1`.
   static const LogicValue one = _FilledLogicValue(_LogicValueEnum.one, 1);
 
-  /// Logical value of `x`
+  /// Logical value of `x`.
   static const LogicValue x = _FilledLogicValue(_LogicValueEnum.x, 1);
 
-  /// Logical value of `z`
+  /// Logical value of `z`.
   static const LogicValue z = _FilledLogicValue(_LogicValueEnum.z, 1);
 
+  /// A zero-width value.
+  static const LogicValue empty = _FilledLogicValue(_LogicValueEnum.zero, 0);
+
   /// The number of bits in this `LogicValue`.
   final int width;
 
@@ -115,16 +118,86 @@ abstract class LogicValue {
   /// Constructs a [LogicValue] from [it].
   ///
   /// The order of the created [LogicValue] will be such that the `i`th entry in
-  /// [it] corresponds to the `i`th bit.  That is, the 0th element of [it] will
-  /// be the 0th bit of the returned [LogicValue].
+  /// [it] corresponds to the `i`th group of bits.  That is, the 0th element of
+  /// [it] will be the least significant chunk of bits of the returned
+  /// [LogicValue].  Bits within each element of [it] are kept in the same
+  /// order as they were originally.
   ///
+  /// For example:
   /// ```dart
-  /// var it = [LogicValue.zero, LogicValue.x, LogicValue.one];
+  /// var it = [LogicValue.zero, LogicValue.x, LogicValue.ofString('01xz')];
   /// var lv = LogicValue.of(it);
-  /// print(lv); // This prints `3b'1x0`
+  /// print(lv); // This prints `6'b01xzx0`
   /// ```
-  static LogicValue of(Iterable<LogicValue> it) => LogicValue.ofString(
-      it.map((e) => e.toString(includeWidth: false)).toList().reversed.join());
+  static LogicValue of(Iterable<LogicValue> it) {
+    var smallBuffer = LogicValue.empty;
+    var fullResult = LogicValue.empty;
+
+    // shift small chunks in together before shifting BigInt's, since
+    // shifting BigInt's is expensive
+    for (final lv in it) {
+      final lvPlusSmall = lv.width + smallBuffer.width;
+      if (lvPlusSmall <= _INT_BITS) {
+        smallBuffer = lv._concatenate(smallBuffer);
+      } else {
+        // only put 64-bit chunks onto `fullResult`, rest onto `smallBuffer`
+        final upperBound =
+            _INT_BITS * (lvPlusSmall ~/ _INT_BITS) - smallBuffer.width;
+        fullResult = lv
+            .getRange(0, upperBound)
+            ._concatenate(smallBuffer)
+            ._concatenate(fullResult);
+        smallBuffer = lv.getRange(upperBound, lv.width);
+      }
+
+      assert(smallBuffer.width <= _INT_BITS,
+          'Keep smallBuffer small to meet invariants and efficiency');
+    }
+
+    // grab what's left
+    return smallBuffer._concatenate(fullResult);
+  }
+
+  /// Appends [other] to the least significant side of `this`.
+  ///
+  /// The new value will have `this`'s current value shifted left by
+  /// the width of [other].
+  LogicValue _concatenate(LogicValue other) {
+    if (other.width == 0) {
+      return this;
+    } else if (width == 0) {
+      return other;
+    }
+
+    final newWidth = width + other.width;
+
+    if (this is _FilledLogicValue &&
+        other is _FilledLogicValue &&
+        other[0] == this[0]) {
+      // can keep it filled
+      return _FilledLogicValue(other._value, newWidth);
+    } else if (newWidth > LogicValue._INT_BITS) {
+      // BigInt's only
+      return _BigLogicValue(_bigIntValue << other.width | other._bigIntValue,
+          _bigIntInvalid << other.width | other._bigIntInvalid, newWidth);
+    } else {
+      // int's ok
+      return _SmallLogicValue(_intValue << other.width | other._intValue,
+          _intInvalid << other.width | other._intInvalid, newWidth);
+    }
+  }
+
+  /// Returns `_value` in the form of a [BigInt].
+  BigInt get _bigIntValue;
+
+  /// Returns `_invalid` in the form of a [BigInt].
+  BigInt get _bigIntInvalid;
+
+  /// Returns `_value` in the form of an [int].
+  int get _intValue;
+
+  /// Returns `_invalid` in the form of an [int].
+  int get _intInvalid;
 
   /// Constructs a [LogicValue] from [it].
   ///
diff --git a/lib/src/values/small_logic_value.dart b/lib/src/values/small_logic_value.dart
index ad5ec151c..f4bd0aa08 100644
--- a/lib/src/values/small_logic_value.dart
+++ b/lib/src/values/small_logic_value.dart
@@ -180,4 +180,18 @@ class _SmallLogicValue extends LogicValue {
               _mask,
           _invalid >> shamt,
           width);
+
+  @override
+  BigInt get _bigIntInvalid =>
+      BigInt.from(_invalid) & _BigLogicValue._maskOfWidth(width);
+
+  @override
+  BigInt get _bigIntValue =>
+      BigInt.from(_value) & _BigLogicValue._maskOfWidth(width);
+
+  @override
+  int get _intInvalid => _invalid;
+
+  @override
+  int get _intValue => _value;
 }
diff --git a/lib/src/values/values.dart b/lib/src/values/values.dart
index 6866bce06..6c982b1a5 100644
--- a/lib/src/values/values.dart
+++ b/lib/src/values/values.dart
@@ -4,7 +4,6 @@
 library values;
 
 import 'package:meta/meta.dart';
-// ignore: unused_import
 import 'package:rohd/rohd.dart';
 
 part 'logic_value.dart';


Test patch:
diff --git a/test/benchmark_test.dart b/test/benchmark_test.dart
index b79acdd09..d463769ef 100644
--- a/test/benchmark_test.dart
+++ b/test/benchmark_test.dart
@@ -9,10 +9,21 @@
 ///
 
 import 'package:test/test.dart';
-import '../benchmark/benchmarks.dart';
+
+import '../benchmark/byte_enable_benchmark.dart';
+import '../benchmark/logic_value_of_benchmark.dart';
+import '../benchmark/pipeline_benchmark.dart';
 
 void main() {
   test('pipeline benchmark', () async {
     await PipelineBenchmark().measure();
   });
+
+  test('logic value of benchmark', () {
+    LogicValueOfBenchmark().measure();
+  });
+
+  test('byte enable benchmark', () {
+    ByteEnableBenchmark().measure();
+  });
 }
diff --git a/test/logic_value_test.dart b/test/logic_value_test.dart
index 1047ba418..23a01580d 100644
--- a/test/logic_value_test.dart
+++ b/test/logic_value_test.dart
@@ -267,6 +267,13 @@ void main() {
           equals(LogicValue.of([LogicValue.zero, LogicValue.zero])));
     });
   });
+
+  test('LogicValue.of example', () {
+    final it = [LogicValue.zero, LogicValue.x, LogicValue.ofString('01xz')];
+    final lv = LogicValue.of(it);
+    expect(lv.toString(), equals("6'b01xzx0"));
+  });
+
   group('unary operations (including "to")', () {
     test('toMethods', () {
       expect(
diff --git a/test/swizzle_test.dart b/test/swizzle_test.dart
index 2f6ddf062..88beba08c 100644
--- a/test/swizzle_test.dart
+++ b/test/swizzle_test.dart
@@ -37,7 +37,7 @@ void main() {
           equals(LogicValue.ofString('zx01')));
     });
   });
-  group('LogicValue', () {
+  group('LogicValue of multi-bit', () {
     test('simple swizzle', () {
       expect([LogicValue.ofString('10'), LogicValue.ofString('xz')].swizzle(),
           equals(LogicValue.ofString('10xz')));
@@ -47,6 +47,95 @@ void main() {
       expect([LogicValue.ofString('10'), LogicValue.ofString('xz')].rswizzle(),
           equals(LogicValue.ofString('xz10')));
     });
+
+    test('64-bit swizzle', () {
+      expect(
+          List.generate(
+              32,
+              (index) => index.isEven
+                  ? LogicValue.ofString('10')
+                  : LogicValue.ofString('xz')).swizzle(),
+          equals(LogicValue.ofString('10xz' * 16)));
+    });
+    test('>64-bit swizzle single concat', () {
+      final str1 = '10xz' * 16;
+      final str2 = 'xz10' * 10;
+      expect([LogicValue.ofString(str2), LogicValue.ofString(str1)].swizzle(),
+          equals(LogicValue.ofString(str2 + str1)));
+    });
+    test('>64-bit swizzle', () {
+      expect(
+          List.generate(
+              40,
+              (index) => index.isEven
+                  ? LogicValue.ofString('10')
+                  : LogicValue.ofString('xz')).swizzle(),
+          equals(LogicValue.ofString('10xz' * 20)));
+    });
+
+    group('variety of sizes', () {
+      test('smaller', () {
+        final bits = ['0', '1', 'x', 'z'];
+        final swizzleStrings = List.generate(
+            100,
+            (index) =>
+                bits[index % bits.length] * (index % 17) +
+                bits[(index + 1) % bits.length] * (index % 2));
+        expect(LogicValue.of(swizzleStrings.map(LogicValue.ofString)),
+            equals(LogicValue.ofString(swizzleStrings.reversed.join())));
+      });
+
+      test('larger', () {
+        final bits = ['0', '1', 'x', 'z'];
+        final swizzleStrings = List.generate(
+            1000,
+            (index) =>
+                bits[index % bits.length] * (index % 71) +
+                bits[(index + 1) % bits.length] * (index % 2));
+        expect(LogicValue.of(swizzleStrings.map(LogicValue.ofString)),
+            equals(LogicValue.ofString(swizzleStrings.reversed.join())));
+      });
+    });
+
+    group('filled', () {
+      test('simple swizzle', () {
+        expect(
+            [LogicValue.ofString('1' * 4), LogicValue.ofString('1' * 4)]
+                .swizzle(),
+            equals(LogicValue.ofString('1' * 8)));
+      });
+      test('big swizzle', () {
+        expect(List.generate(100, (index) => LogicValue.one).swizzle(),
+            equals(LogicValue.ofString('1' * 100)));
+      });
+      test('0-width both swizzle', () {
+        expect([LogicValue.ofString(''), LogicValue.ofString('')].swizzle(),
+            equals(LogicValue.ofString('')));
+      });
+      test('0-width lhs swizzle', () {
+        expect([LogicValue.ofString(''), LogicValue.ofString('111')].swizzle(),
+            equals(LogicValue.ofString('111')));
+      });
+      test('0-width rhs swizzle', () {
+        expect([LogicValue.ofString('111'), LogicValue.ofString('')].swizzle(),
+            equals(LogicValue.ofString('111')));
+      });
+    });
+
+    group('non-filled', () {
+      test('0-width both swizzle', () {
+        expect([LogicValue.ofString(''), LogicValue.ofString('')].swizzle(),
+            equals(LogicValue.ofString('')));
+      });
+      test('0-width lhs swizzle', () {
+        expect([LogicValue.ofString(''), LogicValue.ofString('1zx0')].swizzle(),
+            equals(LogicValue.ofString('1zx0')));
+      });
+      test('0-width rhs swizzle', () {
+        expect([LogicValue.ofString('1zx0'), LogicValue.ofString('')].swizzle(),
+            equals(LogicValue.ofString('1zx0')));
+      });
+    });
   });
 
   group('Logic', () {