[libre-riscv-dev] [Bug 173] dynamic partitioned "shift"

[bugzilla-daemon at libre-riscv.org]

http://bugs.libre-riscv.org/show_bug.cgi?id=173

--- Comment #32 from Luke Kenneth Casson Leighton <lkcl at lkcl.net> ---
just doing some review / commenting / etc.  this one's quite important:

diff --git a/src/ieee754/part_shift/part_shift_dynamic.py
b/src/ieee754/part_shift/part_shift_dynamic.py
index 5dd72ed..95c07d6 100644
--- a/src/ieee754/part_shift/part_shift_dynamic.py
+++ b/src/ieee754/part_shift/part_shift_dynamic.py
@@ -63,12 +63,14 @@ class PartitionedDynamicShift(Elaboratable):
         for i in range(len(b_intervals)):
             mask = Signal(b_intervals[i].shape(), name="shift_mask%d" % i,
                           reset_less=True)
-            bits = []
+            bits = Signal(gates.width-i+1, name="bits%d" % i, reset_less=True)
+            bl = []
             for j in range(i, gates.width):
-                if bits:
-                    bits.append(~gates[j] & bits[-1])
+                if bl:
+                    bl.append(~gates[j] & bits[j-i-1])
                 else:
-                    bits.append(~gates[j])
+                    bl.append(~gates[j])
+            comb += bits.eq(Cat(*bl))
             comb += mask.eq(Cat((1 << min_bits)-1, bits)
                             & ((1 << max_bits)-1))
             shifter_masks.append(mask)


if you "accumulate" something in a chain (bits, in this case), then assign it
later (in mask), what you find is that the entire *chain* is duplicated...
*multiple times*.

by assigning each bit of "bits" to a Signal, and using the *signal* - not the
chain - you don't end up with a massive cascading code-repetition.

i deployed this trick in another area of the code as well.  this one's
quite important: it looks "really simple" to just do bits = bits & foo
or bits.append(input & bits[-1]) however it's a pattern that really needs
to be avoided.

yes, found it:

            element = Mux(gates[i-1], masked, element)
            elmux = Signal(b_intervals[i].shape(), name="elmux%d" % i,
                          reset_less=True)
            comb += elmux.eq(element)
            element = elmux

that was another one where there was an accidental "repeated chain", this
time from the input to Mux().


i'm trying to identify where the max() thing can be put back in.  i think
it's where         shiftbits = math.ceil(math.log2(width))

that needs to be inside the loop, and it needs to be
     shiftbits = math.ceil(math.log2(width-intervals[i].start))

something like that.

then, the b partial... shifter... needs to be set not to this:

               comb += shifter.eq(element)

it needs to be

    m.If(element > shiftbits)
         comb += shifter.eq(shiftbits)
    m.Else()
         comb += shifter.eq(element)

i think that's it.

the reason is, there's absolutely no point trying to shift the data beyond
the limit of the partial.

also, partial needs to be limited to width-intervals[i].start

the last column is only 8 bits, for example, so why is the partial result
always set to 64 bit?

-- 
You are receiving this mail because:
You are on the CC list for the bug.