Repository: intel/rohd (dart)

Issue:
Logic operation fails on LogicValue wider than 127
https://github.com/intel/rohd/blob/344db0e18fa702eda056ed1ddbcda7717e154c17/lib/src/values/logic_value.dart#L783

The conversion to Int rather than BigInt means that logic operations can fail when LogicValues are wide.
The limit appears to be 128.

Pull request description:
<!-- Please make sure you check out the contribution guidelines before submitting a pull request! -->

## Description & Motivation

Adds two tests for wide bitvector comparisons.  Adds a fix for one of them.
The remaining problem is that shifting a 1 into the signed position and then converting to bigint results in a negative number regardless.

## Related Issue(s)

Fix #299

## Testing

Created two tests that do comparisons.  One just breaks the length check.  The other puts a 1 in the sign location just before conversion to BigInt (the fix for the first).

## Backwards-compatibility

> Is this a breaking change that will not be backwards-compatible? If yes, how so?

The change is in the core logic comparison operation so more tests may be needed.

## Documentation

> Does the change require any updates to documentation? If so, where? Are they included?

No

Patch:
diff --git a/lib/src/values/big_logic_value.dart b/lib/src/values/big_logic_value.dart
index 24258369e..f76aab2f9 100644
--- a/lib/src/values/big_logic_value.dart
+++ b/lib/src/values/big_logic_value.dart
@@ -45,7 +45,8 @@ class _BigLogicValue extends LogicValue {
   static final Map<int, BigInt> _masksOfWidth = {};
   static BigInt _maskOfWidth(int width) {
     if (!_masksOfWidth.containsKey(width)) {
-      _masksOfWidth[width] = (BigInt.one << width) - BigInt.one;
+      _masksOfWidth[width] =
+          ((BigInt.one << width) - BigInt.one).toUnsigned(width);
     }
     return _masksOfWidth[width]!;
   }
@@ -60,8 +61,8 @@ class _BigLogicValue extends LogicValue {
       : assert(width > LogicValue._INT_BITS,
             '_BigLogicValue should only be used for large values'),
         super._(width) {
-    _value = _mask & value;
-    _invalid = _mask & invalid;
+    _value = (_mask & value).toUnsigned(width);
+    _invalid = (_mask & invalid).toUnsigned(width);
 
     assert(
         allowInefficientRepresentation ||
diff --git a/lib/src/values/logic_value.dart b/lib/src/values/logic_value.dart
index fa7462204..ab2b6a1c7 100644
--- a/lib/src/values/logic_value.dart
+++ b/lib/src/values/logic_value.dart
@@ -18,6 +18,8 @@ typedef LogicValues = LogicValue;
 ///
 /// Each bit of [LogicValue] can be represented as a [LogicValue]
 /// of `0`, `1`, `x` (contention), or `z` (floating).
+///
+/// [LogicValue] is unsigned.
 @immutable
 abstract class LogicValue implements Comparable<LogicValue> {
   /// The number of bits in an int.
@@ -839,25 +841,21 @@ abstract class LogicValue implements Comparable<LogicValue> {
 
   /// Addition operation.
   ///
-  /// WARNING: Signed math is not fully tested.
   // ignore: avoid_dynamic_calls
   LogicValue operator +(dynamic other) => _doMath(other, (a, b) => a + b);
 
   /// Subtraction operation.
   ///
-  /// WARNING: Signed math is not fully tested.
   // ignore: avoid_dynamic_calls
   LogicValue operator -(dynamic other) => _doMath(other, (a, b) => a - b);
 
   /// Multiplication operation.
   ///
-  /// WARNING: Signed math is not fully tested.
   // ignore: avoid_dynamic_calls
   LogicValue operator *(dynamic other) => _doMath(other, (a, b) => a * b);
 
   /// Division operation.
   ///
-  /// WARNING: Signed math is not fully tested.
   // ignore: avoid_dynamic_calls
   LogicValue operator /(dynamic other) => _doMath(other, (a, b) => a ~/ b);
 
@@ -881,7 +879,7 @@ abstract class LogicValue implements Comparable<LogicValue> {
     }
     if (this is BigInt ||
         this is _BigLogicValue ||
-        (this is _FilledLogicValue && width >= _INT_BITS)) {
+        (this is _FilledLogicValue && width > _INT_BITS)) {
       final a = toBigInt();
       return LogicValue.ofBigInt(op(a, width) as BigInt, width);
     } else {
@@ -925,12 +923,12 @@ abstract class LogicValue implements Comparable<LogicValue> {
       return LogicValue.filled(other.width, LogicValue.x);
     }
 
-    if (this is _BigLogicValue || other is BigInt || other is _BigLogicValue) {
+    if (width > _INT_BITS || (other is LogicValue && other.width > _INT_BITS)) {
       final a = toBigInt();
       final b = other is BigInt
           ? other
           : other is int
-              ? BigInt.from(other)
+              ? BigInt.from(other).toUnsigned(_INT_BITS)
               : other is LogicValue
                   ? other.toBigInt()
                   : throw Exception(
@@ -968,28 +966,24 @@ abstract class LogicValue implements Comparable<LogicValue> {
 
   /// Less-than operation.
   ///
-  /// WARNING: Signed math is not fully tested.
   LogicValue operator <(dynamic other) =>
       // ignore: avoid_dynamic_calls
       _doCompare(other, (a, b) => (a < b) as bool);
 
   /// Greater-than operation.
   ///
-  /// WARNING: Signed math is not fully tested.
   LogicValue operator >(dynamic other) =>
       // ignore: avoid_dynamic_calls
       _doCompare(other, (a, b) => (a > b) as bool);
 
   /// Less-than-or-equal operation.
   ///
-  /// WARNING: Signed math is not fully tested.
   LogicValue operator <=(dynamic other) =>
       // ignore: avoid_dynamic_calls
       _doCompare(other, (a, b) => (a <= b) as bool);
 
   /// Greater-than-or-equal operation.
   ///
-  /// WARNING: Signed math is not fully tested.
   LogicValue operator >=(dynamic other) =>
       // ignore: avoid_dynamic_calls
       _doCompare(other, (a, b) => (a >= b) as bool);
@@ -1040,19 +1034,30 @@ abstract class LogicValue implements Comparable<LogicValue> {
 
     dynamic a;
     dynamic b;
-    if (this is _BigLogicValue || other is BigInt || other is _BigLogicValue) {
+    if (width > _INT_BITS || (other is LogicValue && other.width > _INT_BITS)) {
       a = toBigInt();
       b = other is BigInt
           ? other
           : other is int
-              ? BigInt.from(other)
+              ? BigInt.from(other).toUnsigned(_INT_BITS)
               : other is LogicValue
                   ? other.toBigInt()
                   : throw Exception(
                       'Unexpected big type: ${other.runtimeType}.');
     } else {
-      a = toInt();
-      b = other is int ? other : (other as LogicValue).toInt();
+      if (width < _INT_BITS) {
+        a = toInt();
+        b = other is int ? other : (other as LogicValue).toInt();
+      } else {
+        // Here we now know: width == _INT_BITS
+        final ai = toInt();
+        final bi = other is int ? other : (other as LogicValue).toInt();
+        if ((ai < 0) || (bi < 0)) {
+          final abig = LogicValue.ofBigInt(BigInt.from(ai), _INT_BITS + 1);
+          final bbig = LogicValue.ofBigInt(BigInt.from(bi), _INT_BITS + 1);
+          return abig._doCompare(bbig, op);
+        }
+      }
     }
     return op(a, b) ? LogicValue.one : LogicValue.zero;
   }
diff --git a/lib/src/values/small_logic_value.dart b/lib/src/values/small_logic_value.dart
index c2939e88d..aaaba70dc 100644
--- a/lib/src/values/small_logic_value.dart
+++ b/lib/src/values/small_logic_value.dart
@@ -102,7 +102,7 @@ class _SmallLogicValue extends LogicValue {
   bool get isFloating => (_invalid == _mask) && (_value == _mask);
 
   @override
-  BigInt toBigInt() => BigInt.from(toInt());
+  BigInt toBigInt() => BigInt.from(toInt()).toUnsigned(width);
 
   @override
   int toInt() {


Test patch:
diff --git a/test/logic_value_width_test.dart b/test/logic_value_width_test.dart
new file mode 100644
index 000000000..efa3123e1
--- /dev/null
+++ b/test/logic_value_width_test.dart
@@ -0,0 +1,166 @@
+// Copyright (C) 2021-2023 Intel Corporation
+// SPDX-License-Identifier: BSD-3-Clause
+//
+// logic_value_width_test.dart
+// Unit tests for width issues (64-bit boundary) in [LogicValue].
+//
+// 2023 September 17
+// Author: Desmond Kirkpatrick <desmond.a.kirkpatrick@intel.com>
+
+import 'package:rohd/rohd.dart';
+import 'package:test/test.dart';
+
+void main() {
+  test('crash compare', () {
+    final input = Const(BigInt.from(2).pow(128), width: 129);
+    final output = Logic();
+    Combinational([
+      If.block([
+        Iff(input.getRange(0, 128) > BigInt.from(0),
+            [output < Const(1, width: 1)]),
+        Else([output < Const(0, width: 1)]),
+      ])
+    ]);
+  });
+
+  test('bad compare', () {
+    const i = 64;
+    final input = Const(BigInt.from(1) << (i - 1), width: i);
+    final output = Logic();
+    Combinational([
+      If.block([
+        Iff(input > BigInt.from(0), [output < Const(1, width: 1)]),
+        Else([output < Const(0, width: 1)]),
+      ])
+    ]);
+    final b = ~input.eq(0);
+    expect(output.value, equals(b.value));
+  });
+
+  test('big value test', () {
+    expect(
+        LogicValue.ofBigInt(BigInt.zero, 128) +
+            LogicValue.ofBigInt(BigInt.zero, 128),
+        LogicValue.ofInt(0, 128));
+  });
+
+  group('values test', () {
+    for (final len in [63, 64, 65, 66, 67]) {
+      final sslv = LogicValue.ofInt(4, len); // small Int hold Big
+      final bslv = LogicValue.ofInt(-0xFFFF, len); // 18446744073709486081
+      final fslv = LogicValue.ofInt(-2, len); // 18446744073709551614
+
+      final sblv = LogicValue.ofBigInt(BigInt.from(4), len);
+      final bblv = LogicValue.ofBigInt(BigInt.from(-0xFFFF), len);
+      final fblv = LogicValue.ofBigInt(BigInt.from(-2), len);
+
+      test('small Int storage len=$len', () {
+        expect(sslv < bslv, LogicValue.one);
+        expect(bslv < sslv, LogicValue.zero);
+        expect(sslv > bslv, LogicValue.zero);
+        expect(bslv > sslv, LogicValue.one);
+
+        expect(sslv < fslv, LogicValue.one);
+        expect(fslv < sslv, LogicValue.zero);
+        expect(sslv > fslv, LogicValue.zero);
+        expect(fslv > sslv, LogicValue.one);
+
+        expect(bslv < fslv, LogicValue.one);
+        expect(fslv < bslv, LogicValue.zero);
+        expect(bslv > fslv, LogicValue.zero);
+        expect(fslv > bslv, LogicValue.one);
+      });
+
+      test('big Int storage len=$len', () {
+        expect(sblv < bblv, LogicValue.one);
+        expect(bblv < sblv, LogicValue.zero);
+        expect(sblv > bblv, LogicValue.zero);
+        expect(bblv > sblv, LogicValue.one);
+
+        expect(sblv < fblv, LogicValue.one);
+        expect(fblv < sblv, LogicValue.zero);
+        expect(sblv > fblv, LogicValue.zero);
+        expect(fblv > sblv, LogicValue.one);
+
+        expect(bblv < fblv, LogicValue.one);
+        expect(fblv < bblv, LogicValue.zero);
+        expect(bblv > fblv, LogicValue.zero);
+        expect(fblv > bblv, LogicValue.one);
+      });
+
+      test('big math len=$len', () {
+        expect(sslv + fslv, LogicValue.ofInt(2, len));
+        expect(sslv - fslv, LogicValue.ofInt(6, len));
+        expect(fslv - sslv, LogicValue.ofInt(-6, len));
+
+        expect(sslv * fslv, LogicValue.ofInt(-8, len));
+
+        expect(sslv + fslv, LogicValue.ofBigInt(BigInt.from(2), len));
+        expect(sslv - fslv, LogicValue.ofBigInt(BigInt.from(6), len));
+        expect(fslv - sslv, LogicValue.ofBigInt(BigInt.from(-6), len));
+
+        expect(fslv * sslv, LogicValue.ofBigInt(BigInt.from(-8), len));
+
+        expect(sblv + fblv, LogicValue.ofInt(2, len));
+        expect(sblv - fblv, LogicValue.ofInt(6, len));
+        expect(fblv - sblv, LogicValue.ofInt(-6, len));
+
+        expect(sblv * fblv, LogicValue.ofInt(-8, len));
+
+        expect(sblv + fblv, LogicValue.ofBigInt(BigInt.from(2), len));
+        expect(sblv - fblv, LogicValue.ofBigInt(BigInt.from(6), len));
+        expect(fblv - sblv, LogicValue.ofBigInt(BigInt.from(-6), len));
+
+        expect(fblv * sblv, LogicValue.ofBigInt(BigInt.from(-8), len));
+      });
+
+      test('division test len=$len', () {
+        final negsfour = LogicValue.ofInt(-4, len);
+        final negbfour = LogicValue.ofBigInt(BigInt.from(-4), len);
+        final two = LogicValue.ofBigInt(BigInt.from(2), len);
+        expect(negsfour / two, LogicValue.ofInt(-4, len) >>> 1);
+        expect(negbfour / two, LogicValue.ofBigInt(BigInt.from(-4), len) >>> 1);
+      });
+
+      test('modulo test len=$len', () {
+        final negsfive = LogicValue.ofInt(-5, len);
+        final negbfive = LogicValue.ofBigInt(BigInt.from(-5), len);
+        final two = LogicValue.ofBigInt(BigInt.from(2), len);
+        expect(negsfive % two, LogicValue.ofInt(1, len));
+        expect(negbfive % two, LogicValue.ofBigInt(BigInt.from(1), len));
+      });
+
+      test('clog test len=$len', () {
+        final negnum = LogicValue.ofInt(-1, len);
+        expect(negnum.clog2(), LogicValue.ofInt(len, len));
+        for (final l in [1, 2, 3]) {
+          expect((negnum >>> l).clog2(), LogicValue.ofInt(len - l, len));
+        }
+        for (final l in [len - 5, len - 4, len - 3, len - 2]) {
+          final bignum = LogicValue.ofBigInt(BigInt.from(1) << l, len);
+          expect(bignum.clog2(), LogicValue.ofInt(l, len));
+          if (len < 64) {
+            final smallnum = LogicValue.ofInt(1 << l, len);
+            expect(smallnum.clog2(), LogicValue.ofInt(l, len));
+          }
+        }
+        for (final l in [len - 5, len - 4, len - 3]) {
+          final bignum = LogicValue.ofBigInt(BigInt.from(2) << l, len);
+          expect(bignum.clog2().toBigInt(), BigInt.from(l + 1));
+          if (len < 64) {
+            final smallnum = LogicValue.ofInt(2 << l, len);
+            expect(smallnum.clog2(), LogicValue.ofInt(l + 1, len));
+          }
+        }
+        for (final l in [len - 5, len - 4, len - 3]) {
+          final bignum = LogicValue.ofBigInt(BigInt.from(3) << l, len);
+          expect(bignum.clog2(), LogicValue.ofInt(l + 2, len));
+          if (len < 64) {
+            final smallnum = LogicValue.ofInt(3 << l, len);
+            expect(smallnum.clog2(), LogicValue.ofInt(l + 2, len));
+          }
+        }
+      });
+    }
+  });
+}