Improve handling of signed numbers

[fdxmw]

It is easy to make confusing mistakes with signed values in PyRTL. A broken example:

import pyrtl

a = -128
b = 127

# Compute a - b, which should be -255.
a_const = pyrtl.Const(name='a', val=a, signed=True, bitwidth=8)
b_const = pyrtl.Const(name='b', val=b, signed=True, bitwidth=8)

diff = a_const - b_const
diff.name = 'diff'

sim = pyrtl.Simulation()
sim.step()

print('expected diff is', a - b)

print('actual diff is',
      pyrtl.val_to_signed_integer(sim.inspect('diff'), bitwidth=diff.bitwidth),
      'bitwidth', diff.bitwidth)

When this example is run, it produces:

expected diff is -255
actual diff is 1 bitwidth 9

Note that the actual output has bitwidth 9, which is sufficient to represent -255.

This example is especially confusing because PyRTL has all the information to do the right thing, because we've marked the Consts as signed=True. To fix this example, the user currently has to call signed_sub instead of using the overloaded - operator.

The current approach is error prone because the user has to keep track of signed-ness as values propagate through their circuit, and make sure they call functions like signed_add and val_to_signed_integer at the right times.

It seems better to track signed-ness within WireVector, or within a WireVector subclass, but this raises questions about type promotion - what happens when we add a signed value and an unsigned value? My current intuition is to match Verilog's promotion behavior, but we should think about this more.

See also #417

[gaborszita]

I was looking into this and noticed that this also causes issues with comparison operators. For example:

import pyrtl

a = -128
b = 127

a_const = pyrtl.Const(name='a', val=a, signed=True, bitwidth=8)
b_const = pyrtl.Const(name='b', val=b, signed=True, bitwidth=8)

comparison = a_const < b_const
comparison.name = 'comparison'

sim = pyrtl.Simulation()
sim.step()

print('expected comparison is', a < b)

print('actual comparison is',
      pyrtl.val_to_signed_integer(sim.inspect('comparison'), bitwidth=comparison.bitwidth),
      'bitwidth', comparison.bitwidth)

I think tracking the signed-ness within WireVector and matching Verilog's type promotion behavior is a good solution, I can work on this if it is finalized that this is what we want to do.

[fdxmw]

Yes, this issue applies to comparison operators as well. Ideally, PyRTL would automatically call signed_lt when comparing two signed WireVectors with __lt__.

Tracking signed-ness within WireVector (or maybe a subclass?) sounds good idea to me, but it depends a lot on whether we can maintain backwards compatibility, and the complexity of the implementation. We should also check if @timsherwood is okay with such a change.

[fdxmw]

This issue came up again in the context of pull request #475 (Pyrtl floating point library), which considered adding a FloatWireVector class. We currently think we need a more complete solution to this problem that considers all the different types involved (signed/unsigned integers, floats, other future types?), with a consistent plan for how these types interact (coercion, promotion, etc), especially when different bitwidths are involved.

Without some kind of "unified WireVector type philosophy," it's too easy to end up with in confusing situations where bits are interpreted incorrectly. Gabor points out this example:

WireVector a
FloatWireVector b
a + b

If we aren't careful, this can easily invoke WireVector.__add__, which will reinterpret b as an unsigned integer, which is almost certainly the wrong thing to do.