Re: [myhdl-list] Incorrect conclusions in TSConIT paper

[Christopher F. <chr...@gm...>]

On 6/22/2012 9:57 AM, Jan Decaluwe wrote:
> The author presents the following example:
>
> --
> from myhdl import *
>
> def firfilt (sigin, sigout, coefs, clk):
>       buffer = Signal(intbv(0)[len(sigin)*len(coefs)])
>       inlen = len(sigin)
>       saved_coefs = coefs
>       colen = len(saved_coefs)
>
>       @always(clk.posedge)
>       def logic():
>           buffer.next[inlen:] = sigin
>           buffer.next[:inlen] = buffer[inlen * (colen-1):]
>           tmp = 0
>           for index in range(colen):
>               mult = saved_coefs[inlen*(index+1):inlen*index] * \
>                      buffer[inlen*(index +1): inlen*index]
>               tmp = tmp + sigin
>           sigout.next = tmp
>
>       return logic

The author did a poor job picking their examples.  I say
this because I don't think their VHDL example is correct,
either.  They don't provide enough information but for the
VHDL example to be correct the instantiator would need to
take care of the output word size to handle the bit growth
as a result of the multiplies and additions (or limit the
input range).  I think if they would have verified the VHDL
they would have found the VHDL implementation did not meet
their requirements (or any requirements for a FIR filter).

Also, I don't think the author provides the description of
the RAM/ROM for the coefficients in the VHDL example.  The
VHDL example is not a fair comparison because it would need
additional description, somewhere, to define the coefficient
array.  Where as the MyHDL version the coefficient array
description is embedded!

Here is my version of the FIR filter in MyHDL that matches
the simplified templated:

     from myhdl import *
     def firfilt(sig_in, sig_out, coef, clk):
         buffer = [Signal(intbv(0, min=sig_in.min, max=sig_in.max)) \
                   for ii in range(len(coef))]
         coef = tuple(coef)
         mshift = len(sig_in)-1
         @always(clk.posedge)
         def hdl_sop():
             sop = 0
             # Note this adds an extra delay! (Group delay N/2+2)
             for ii in range(len(coef)):
                 buffer[ii].next = sig_in if ii == 0 else buffer[ii-1]
                 c = coef[ii]
                 sop = sop + (buffer[ii] * c)
             sig_out.next = (sop >> mshift)

         return hdl_sop

The testbench can be found here:
https://bitbucket.org/cfelton/examples/src/tip/firfilt/test_firfilt.py

and some plots of the working filter.
http://flic.kr/p/cko2SY
http://flic.kr/p/cko2Tj

Note:  This example is not reasonable for most implementations.
A very large combinational path is created for the multiplies
and accumulate (MAC).  Most implementations would break this up
some how, pipelining, multiple clocks ticks (flow control), etc.

To be honest, I ran into a couple bumps.  I had created the
following version first.

         @always(clk.posedge)
         def hdl_sop():
             buffer[0].next = sig_in
             sop = sig_in * coef[0]
             for ii in range(1, len(coef)):
                 buffer[ii].next = buffer[ii-1]
                 sop = sop + (buffer[ii-1] * coef[ii])
             sig_out.next = (sop >> mshift)

My first off the cuff, solution was to avoid an extra delay by
using the sig_in and not the registered version.  But this didn't
follow the list of signals (LoS) memory template and was not
convertible.  This can be fixed by not including the LoS access
in the expressions.

         @always(clk.posedge)
         def hdl_sop():
             buffer[0].next = sig_in
             c = coef[0]
             sop = sig_in * c
             for ii in range(1, len(coef)):
                 buffer[ii].next = buffer[ii-1]
                 c = coef[ii]
                 sop = sop + (buffer[ii-1] * c)
             sig_out.next = (sop >> mshift)

This is one of those rules that you have to remember but there
is good reason.  You are getting the coefficient description
(in this case a ROM) without needing a completely separate
generator, the description is conveniently embedded.  Well
almost conveniently, you do have to access the /tuple of ints/
(ToI) separately.  If the ToI is not access separately the
conversion code would have to infer signals between the ROM
description and the expressions, which might not be desirable
(at least that is my understanding?).

The one thing the authors inadvertently demonstrate, is the
*importance* of verifying the design even if the example isn't
the main focus.  As you point out, how can you make any kind
of reasonable argument if one language is more /concise/,
/comprehensible/, or /reusable/ if the example is not even
functional!


Regards,
Chris