Another MyHDL VHDL conversion bug

The following line

self.res_o.next =  concat( modbv(0)[31:], self.op1_i < self.op2_i )

is converted to this VHDL code:
self_res_o_num <= unsigned'(unsigned'("0000000000000000000000000000000") & (self_op1_i_num < self_op2_i_num));

This leads to an VHDL converison error because the comparison returns a boolean and not a std_logic.
The right code must look like
self_res_o_num <= unsigned'(unsigned'("0000000000000000000000000000000") & stdl(self_op1_i_num < self_op2_i_num));

Current workaround is to store the result of the comparion in a temporary variable in MyHDL:

 t_comp = self.op1_i < self.op2_i
 self.res_o.next =  concat( modbv(0)[31:], t_comp  )

This is converted to:

variable t_comp: std_logic;
....
 t_comp := stdl(self_op1_i_num < self_op2_i_num);
 self_res_o_num <= unsigned'(unsigned'("0000000000000000000000000000000") & t_comp);

Yes,
I saw that error in the generated VHDL yesterday too, and modified the code to deal with that while focusing on the ShadowSignal issue. I forgot about this then …
You can replace the offending line:
self.res_o.next = concat( modbv(0)[31:], self.op1_i < self.op2_i )
by:
self.res_o.next = self.op1_i < self.op2
It will convert to:
self_res_o_num <= to_unsigned(self_op1_i_num < self_op2_i_num, 32);
(which I hope will be correct as well, at least the Sigasi editor doesn’t flag an error there)

Regards,
Josy

Yes indeed.
Unfortunately I found already another VHDL conversion bug, this time regarding resizing of signed signals:

My code contains a line

def get_SB_immediate(instr):
    return concat(instr[31],instr[7],instr[31:25],instr[12:8],intbv(0)[1:])

......
    rs1_imm_value=Signal(modbv(0)[self.xlen:])
......
         rs1_imm_value.next = get_SB_immediate(self.word_i).signed()

get_SB_immediate returns a 13 Bit Value. self.xlen is 32. So the assignment should sign extend the 13 Bit value. In MyHDL simulation it works correct. The signal assignment is converted to

rs1_imm_value <= resize(unsigned(signed(MYHDL82_get_SB_immediate(self_word_i_num))), 32);

which is zero extending the value. The unsigned cast need to be outside of the resize:

rs1_imm_value <= unsigned(resize(signed(MYHDL82_get_SB_immediate(self_word_i_num)), 32));

I have not yet found a code variant which returns the correct result.

@ThomasHornschuh Thanks! Please please please put this into a pull request with a failing test case. If you don’t want to do the fix, then it would be fantastic if you can just produce a failing test case inside the test framework, and allow editing of your pull request branch so we can push the fix.

@ThomasHornschuh somewhere in here: https://github.com/myhdl/myhdl/tree/master/myhdl/test/conversion/toVerilog (and the equivalent in VHDL). Add it to an appropriate test file or create another is you feel is best.

I will take a look and try to create a test in the next days. I’m afraid that it will take to much time for me to try fixing it on my own, given the fact that I’m just learning Python because of MyHDL.

The first “bug” I found in a hindsight may not be a bug, because I learned in the meantime that MyHDL conversion is requiring VHDL-2008. VHDL-2008 allows coercion between logic types and booleans. So I will check if the generated code runs with std=08 in GHDL.
The second example is indeed an error, I have already checked that the VHDL code behaves different from the Python code (zero extension vs. sign extension…)

BTW: I quick look have shown me that you use the verify method in your tests. I was not able yet to get verify running, maybe somebody can give me a hint, how to set it up correctly vor GHDL.

@ThomasHornschuh Do you have py.test and GHDL installed? Easiest to go and ask on gitter, which is a bit more interactive.

There’s a bug where unsigned is added by the converter while it should not. I don’t remember where it is.
Not sure this applies here.
I’ll search for the culprit line.

Beware that MyHDL consider everything as unsigned by default. For example, a slice of a signal is considered as unsigned, whatever the source signal is.

@DrPi, @hgomersall
I was busy with other things, sorry for not answering anymore. I have no a bit more time to work on my MyHDL project. So hoepfully I will spend some time with this topic soon.

@DrPi “myhdl considers everything as unsinged by default”, this statement is incorrect. You are correct that a slice is considered unsigned, which makes sense. A slice of a bit vector is a special case.

If you only code using bit vectors (intbv(0)[32:0]), then correct these are always unsigned. See @jandecaluwe these ints were made for counting for some insight.

So, what is not unsigned by default ?

I made some testing to understand a bit better how MyHDL works, because the documentation is not clear for me.
To my understanding MyHDL integers are using two-complement logic. The question if a value is considered “signed” or “unsigned” is only relevant for sign extension: When a signed int/modbv is assigned to a longer int/modbv its highest bit will be extended into the additional bits.

According to my experiments this only happens when there is a defined length:

>>> y=modbv(0)[32:] # Our "target"
>>> u=intbv(5)  # intbv with undefined length
>>> len(u)
0
>>> y[:]=u 
>>> u
intbv(5)
>>> y[:]=u.signed()  # Signed makes no difference here
>>> u
intbv(5)
>>> d=intbv(5)[3:]    # intbv with a defined length
>>> y[:]=d
>>> y
modbv(5L)
>>> y[:]=d.signed()  # Now it will sign extend the highest bit of d 
>>> y
modbv(4294967293L)
>>> bin(y)
'11111111111111111111111111111101'
>>> bin(y.signed())   # This is surprising, signed() seems to short return the shortest possible representation
'101'
>>> y.signed()  
intbv(-3L)

If a intbv is initialized with a negative value it is considered signed by default

>>> n=intbv(-1)
>>> bin(n)
'1'
>>> y[:]=n
>>> bin(y)
'11111111111111111111111111111111'

What I have not found out yet how to legally assign a negative value to an intbv of a defined bit length

>>> z=intbv(min=-2**31,max=2**31)  # intbv without defined bit length
>>> z.max
2147483648
>>> z.min
-2147483648
>>> len(z) # The min/max constraints implicitly define a bit length
32
>>> z[:]=n  # Works
>>> z
intbv(-1L)
>>> z=intbv(min=-2**31,max=2**31)[32:] # Supposed to be 32 Bit signed integer
>>> z.max  # ???
4294967296
>>> z.min # ???
0
>>> z[:]=n
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python2.7/dist-packages/myhdl/_intbv.py", line 162, in __setitem__
    self._handleBounds()
  File "/usr/local/lib/python2.7/dist-packages/myhdl/_intbv.py", line 79, in _handleBounds
    (self._val, self._min))
ValueError: intbv value -1 < minimum 0

>>> xx=intbv(d.signed())  # works as expcted 
>>> xx
intbv(-3L)
>>> xx=intbv(d.signed())[32:] # strange...
>>> xx
intbv(4294967293L)
>>> bin(xx)
'11111111111111111111111111111101'

So my observation is so far:
intbv are considered signed when they are either initialized with a negative value or get a negative min constraint.
The signed() method requires the int/modbv to have a defined bit length to be useful. In this case the highest bit is considered as sign bit.

Maybe I took the wrong conclusions out of my experiments.

Everything is coherent in your tests.

  1. When you specify min and max values, you give a length to the intbv. No need to specify a length with [length:].
  2. intbv(min_val)[length:] is some sort of shortcut for intbv(min_val, min=0, max=2**length)
  3. Slices are considered unsigned.
    So intbv(init_val, min=min_val, max=max_val)[length:] creates a signed intbv (with some length) then slices it with a length of length. The result is an unsigned intbv.

As a conclusion, When creating signed intbv do not use slicing. Use the declaration with min and max.
I personally created a utility function generating a signed intbv. Something like :

def SignedBitVector(reset_val, length) :
    return intbv(reset_val, min=-(2**(length-1)), max=2**(length-1))

I feel that the shortform: intbv(v)[n:] should never have existed (although I use it all the time)