Replace BinaryValue with LogicArray

[aubindetrez]

Replace BinaryValue with LogicArray like mentioned in issue #608 and PR #706.
All internal references to BinaryValue are removed.

Please this is a Breaking Changes - do not merge as-is, further discussions are required.

I think BinaryValue should be reintroduced for compatibility reason but marked deprecated. (And next major release it can be definitely removed). Let me know what you think.

Status / To dos:

• Remove BinaryValue and internal references to it.
• Pass all tests
  • See list of tests below
• Implement all missing operators
  • See list below
• Make type conversion more explicit LogicArray: Make type conversions more explicit #3418
  • Remove implicit conversion to unsigned integers (Replace BinaryValue with LogicArray  #3244 (comment))
• Remove Bit
• Construct LogicArray from String
• Construct LogicArray from Iterable of Logic
• Construct LogicArray from integer with explicit endianness
• Convert LogicArray to String (binary representation)
• Convert LogicArray to Integer and explicitly specify endianness and representation
• get / set an element
• get the length
• bitwise logic
• Explicit arithmetic shifts
• Explicit logical shifts
• No arithmetic operators with implicit conversion to int...
• get 'slices' (value = signal[3:0])
• set 'slices' (signal[3:0] = value)
• Have comparable performance to BinaryValue
  • Refactor LogicArray according to LogicArray: higher performance implementation #2769
  • Note: We may try different implementation in other branches, and merge the best one in this PR.
  • See below for the current performance compared to BinaryValue
• Proof of concept: Port one "big cocotb project" to use LogicArray instead of BinaryValue
• Add meaningful error message to help with porting big code bases
• cocotb/cocotb/handle.py uses downto by default, do we want to add an option for using to ?
• Update the documentation (There are many references to BinaryValue)

LogicArray's operators compared to BinaryValue

The list of operators have been moved to: https://gist.github.com/aubindetrez/16ffe3b172ff0edd138001d61437d4ef (to improve clarity.

Failing tests

• pytest cocotb/tests/pytests/test_logic_array.py depends on BinaryValue
• Remove pytest cocotb/tests/pytest/test_binary_value.py
• Test test_matrix_multiplier_icarus from cocotb/cocotb/examples/matrix_multiplier/tests/test_matrix_multiplier.py with error ModuleNotFoundError: No module named 'cocotb.binary'
• Test test_matrix_multiplier_ghdl
• Test issue_142_overflow_error from cocotb/tests/test_cases/issue_142/issue_142.py with error Test failed with RANDOM_SEED=1694419089
• Test test_assigning_structure from cocotb/tests/test_cases/test_cocotb/test_deprecated.py with error Test failed with RANDOM_SEED=1694419168
• Test test_force_release from cocotb/tests/test_cases/test_force_release/test_force_release.py with error Test failed with RANDOM_SEED=1694419237
• Test test_my_design.my_first_test from cocotb/cocotb/examples/doc_examples/quickstart with error AssertionError: my_signal_2[0] is not 0!
• Test test_my_design.my_second_test from cocotb/cocotb/examples/doc_examples/quickstart with error AssertionError: my_signal_2[0] is not 0!
• issue_142_overflow_error
• test_force_release

*If checked, the test passed at least locally (nox + pytest) but for the CI you have to scroll down this page.

Performance

• Performance results before any optimization: LogicArray is 5x slower than BinaryValue for MatrixMultiplication https://gist.github.com/aubindetrez/bb6e311bbfe8044a839ee22f98ae7b18
• Experiment 1: 'Cache' self.integer: LogicArray is 2.7x slower than BinaryValue for MatrixMultiplication

[aubindetrez]

@ktbarrett I would greatly appreciate your feedback on this.

[ktbarrett]

I'm not against this change. I appreciate the work! I think this PR should come in, but maybe we should fix a few critical bugs and do a 1.8 release before this comes in.

There is some additional development on these types that needs to occur, namely for performance (#2769). Also we need to consider removing Bit (#2666), as I don't think it's really useful.

[ktbarrett]

No need to add this since it's been deprecated.

[aubindetrez]

The idea was to help user switch to LogicArray with a (hopefully) straightforward error.
While migrating big projects it could save even more time.

[ktbarrett]

You should install pre-commit and run pre-commit on your commits. This should make sure that your commits will pass all the lint checks.

pip install pre-commit
pre-commit install

You can run the checks at any time using

pre-commit run -a

[teobiton]

Hi,

Regarding the status, performance analysis is something to consider as well.
First observations by comparing profiling from the matrix multiplier with BinaryValue and with LogicArray, is that there is a huge drop in performance.

Based on current master and on the remove_binary_value branch:

Branch	master	remove_binary_value
Most time consuming function	binary.py:249(_convert_from_unsigned)	logic_array.py:202(integer)
Total time spent (s)	1.294	16.51
Cumulative time spent (s)	1.993	31.4

The source of the performance drop comes from the way the value is handled in LogicArray: each time the integer representation is needed, the array is looped over to build it.
As mentionned in #2769, it would be best to keep to python native types for better performance.

[aubindetrez]

Yes performance is an issue too and it is in the to do list at the top of the issue, thanks for mentioning it.
Looks like #2769 is about refactoring LogicArray with performance in mind.

Optimizing performance of something not functional could be very error prone. Without the full picture we could be missing optimization opportunities or optimize out things we need. I suggest we get this branch to pass CI first, then refactor LogicArray to improve performance.
Unless we can do both in parallel ?!

EDIT: I assume we want to use LogicArray instead of BinaryValue anyway and that we don't want to introduce a new type.

[aubindetrez]

I tried to "cache LogicArray.integer" in a separate branch (look for "Experiment 1: 'Cache' self.integer" in first top of the PR). I get significant performance improvements with a simple change, but it's nowhere near BinaryValue.

I will keep experimenting with different implementations and share results and code in the first comment, at the top of the PR. If anyone else want to give it a try, you can comment below with your results/branch and I will add it to the list too.

[aubindetrez]

The latest commit passes CI.

FYI: To run python3.7 tests locally, I used nox -s dev_test-3.7 with:

--- a/noxfile.py
+++ b/noxfile.py
@@ -123,7 +123,7 @@ def dev_build(session: nox.Session) -> None:
     session.warn("No building is necessary for development sessions.")
 
 
-@nox.session
+@nox.session(python=["3.10", "3.7", "3.8"])
 def dev_test(session: nox.Session) -> None:
     """Run all development tests as configured through environment variables."""

Just in case someone else needs to debug python3.7 compatibility.

EDIT: I'm currently working on improving the performance and writing new performance benchmarks.

[imphil]

Thanks for your work, @aubindetrez! I just had a chat with @ktbarrett (and he'll probably jump in with more details), but just as a first notice:

LogicArray is (meant to be) not interpreting the data automatically, i.e. it only knows about bits (and Xes, etc.), but not about signed/unsigned, floats, byte ordering, etc. That means we cannot have mathematical operations and the like directly on LogicArray. Instead, users need to explicitly use LogicArray.signed_integer and friends and operate on them.

While looking at this issue I thought about ways to make the API more explicit, please have a look at #3418 and join the discussion.

[ktbarrett]

I think we can also remove Bit as it currently serves no purpose. There was interest by me originally to add more types, but I think we aren't doing that anymore.

[ktbarrett]

Generally what we need is:

• construct from string
• construct from iterable of logic
• construct from integer specifying the endianness (representation can be deduced since we only support two's complement and unsigned, which is a subset)
• conversion to binary string
• conversion to integer specifying the endianness and representation
• sequence-like stuff: get and set elements, iteration, length, blah blah blah
• bitwise logic

What we cannot assume:

• shifts (are they arithmetic or logical)
• any arithmetic operators
• how users want hex and octal representations formatted

[teobiton]

Hi all,

To expand on the performance trials, I implemented an int and string representation for LogicArray that is used as much as possible when valid, i.e not modified by set_item and other arithmetic operations that are implemented for now (see logicarray_perf_repr).

The results are far better than the original implementation but we are nowhere near the results with BinaryValue per the construction of LogicArray inheriting from Array[Logic]. A lot of time is spent rebuilding the array even with the string and int representations (total time 7 seconds for binstr, integer has better results around 4 seconds total time).

[codecov]

Codecov Report

Patch coverage: 47.95% and project coverage change: +0.12% 🎉

Comparison is base (d56692a) 43.78% compared to head (eec5994) 43.91%.
Report is 5 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3244      +/-   ##
==========================================
+ Coverage   43.78%   43.91%   +0.12%     
==========================================
  Files          49       48       -1     
  Lines        8863     8433     -430     
  Branches     2458     2356     -102     
==========================================
- Hits         3881     3703     -178     
+ Misses       4401     4190     -211     
+ Partials      581      540      -41     

Files Changed	Coverage Δ
cocotb/types/logic.py	66.21% <33.33%> (+3.05%)	⬆️
cocotb/types/logic_array.py	53.14% <43.75%> (+8.14%)	⬆️
cocotb/types/range.py	59.30% <50.00%> (+6.86%)	⬆️
cocotb/handle.py	78.70% <80.00%> (-0.17%)	⬇️
cocotb/types/__init__.py	25.00% <100.00%> (ø)

... and 13 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ktbarrett]

@teobiton The next stumbling block is probably the construction of the Range object, which occurs if no range is given. You can test that by creating a Range object once and passing it in the range argument in your benchmark and see if performance improves.

We can then investigate maintaining multiple Range objects as well.

I wonder if we should start from the ground up with performance in mind (at least for the LogicArray class).

[ktbarrett]

@teobiton Actually, I see you are still creating the List[Logic] repr even when being constructing from a string. You should populate the str represent instead of the List[Logic] repr when given a string and create the List[Logic] impl on demand. Most of the performance issues come from constructing this list. It requires the creation of N Logic values and a list to hold them.

An easy way to do this is with getter which encapsulate the creation. The only time you need to set the string and int representations is on construction and I don't know if we need to cache them after that fact, so once they are invalidated on a __setitem__, they are permanently invalidated. Although, you might go super extra and do general-purpose caching on the string and/or int representations like you are doing rn.

A small mock up

class LogicArray:
    @overload
    def __init__(self, value: str):
        ...

    @overload
    def __init__(self, value: int, *, byteorder: str) -> None:
        ...

    @overload
    def __init__(self, value: SizedIterable[LogicConstructible]) -> None:
        ...

    def __init__(self, value, *, byteorder: str):
        self._str_impl_valid = False
        self._list_impl_valid = False
        self._int_impl_valid = False
        if isinstance(value, str):
            self._str_impl = value
            # validate
            self._str_impl_valid = True
        elif isinstance(value, int):
            self._int_impl = value
            self._int_impl_byteorder = byteorder
            # validate
            self._int_impl_valid = True
        else:
            self._list_impl = list(value)
            # validate
            self._list_impl_valid = True

    def __str__(self) -> str:
        if self._str_impl_valid:
            return self._str_impl
        # otherwise construct from int or logic representation

    def as_int(self, *, byteorder: str, signed: bool) -> int:
        if self._int_impl_valid and self._int_impl_byteorder == byteorder:
            # might need to work out if two's complement is needed if 'signed' is False and '_int_impl' is negative
            return self._int_impl
        # otherwise construct from str, int, or logic representation

    @property
    def _list_impl(self) -> List[Logic]:
        if self._list_impl_valid:
            return self._list_impl_
        # construct from str or int representation

    def __iter__(self) -> Iterable[Logic]:
        return iter(self._list_impl)

    def __reversed__(self) -> Iterable[Logic]:
        return reversed(self._list_impl)

    def __setitem__(self, key: int, value: LogicConstructable) -> None:
        self._int_impl_valid = False
        self._str_impl_valid = False
        # validate
        return self._list_impl[index]

[ktbarrett]

Where are we with this?

[aubindetrez]

We (I) still need to change the API as discussed by Philipp and you. This will break many tests and examples.

We also need to improve performance further.

I personally don't have a lot of free time right now but I hope it should get better in a few weeks. If anyone else wants to work on it he/she is more than welcome.

[teobiton]

So I picked up things where I left them and tried a new implementation based on your comments @ktbarrett, still on the same branch logicarray_perf_repr). It's not quite done yet.

Do we still want a Range being constructed and inheriting from Array[Logic] ? In my current trial I totally removed them from the class (breaking everything obviously), only building the Logic(_) array when required.

[marlonjames]

@ktbarrett Is this PR superseded by your other handle PRs?

[ktbarrett]

Superceded by #3634.