myhdl / myhdl

The MyHDL development repository
http://www.myhdl.org
GNU Lesser General Public License v2.1
1.04k stars 249 forks source link

Generated verilog does not match Python simulation #287

Open tomtor opened 5 years ago

tomtor commented 5 years ago

I have the following function:

b1 = Signal(intbv()[8:])                                                    
b2 = Signal(intbv()[8:])                                                    
b3 = Signal(intbv()[8:])                                                    
b4 = Signal(intbv()[8:])                                                    
dio = Signal(intbv()[3:])

def get4(boffset, width):                                                   
  return (((b4 << 24) | (b3 << 16) | (b2 << 8) | b1) >> (dio + boffset)) & ((1 << width) - 1) 

This concatenates b1 to b4 and selects width bits from the 32 bit result, in MyHDL Python and in Icarus 9.0 and translates to this verilog fragment

MYHDL132_get4 = ((((((deflate0_0_b4 << 24) | (deflate0_0_b3 << 16)) | (deflate0_0_b2 << 8)) | deflate0_0_b1) >>> 
        (deflate0_0_dio + boffset)) & ((1 << width) - 1));

However, in Icarus 10.0 and in Vivado this does not work, because the result of the shifts remain 8-bit, so only b1 is used!!!

I can fix this for Icarus by:

bb4 = intbv(b4.val)[32:]                                              
bb3 = intbv(b3.val)[24:]                                              
bb2 = intbv(b2.val)[16:]                                              
return (((bb4 << 24) | (bb3 << 16) | (bb2 << 8) | b1) >> (dio + boffset)) & ((1 << width) - 1) 

which produces:

function integer MYHDL132_get4;
    input boffset;
    input width;
    integer width;
    reg [32-1:0] bb4;
    reg [24-1:0] bb3;
    reg [16-1:0] bb2;
begin: MYHDL204_RETURN
    bb4 = deflate0_0_b4[32-1:0];
    bb3 = deflate0_0_b3[24-1:0];
    bb2 = deflate0_0_b2[16-1:0];
    MYHDL132_get4 = ((((((bb4 << 24) | (bb3 << 16)) | (bb2 << 8)) | deflate0_0_b1) >>> 
        (deflate0_0_dio + boffset)) & ((1 << width) - 1));
    disable MYHDL204_RETURN;
end
endfunction

but Vivado complains:

[Synth 8-524] part-select [31:0] out of range of prefix 'deflate0_b4' 

I also tried constructs like:

return concat(b4, b3, b2, b1)(dio + boffset + width, dio + boffset)

but MyHDL complains that it cannot determine the type.

As a last resort I changed the variables to:

  b1 = Signal(intbv()[32:])                                                   
  b2 = Signal(intbv()[24:])                                                   
  b3 = Signal(intbv()[16:])                                                   
  b4 = Signal(intbv()[8:])                                                    

but that is not very elegant.

josyb commented 5 years ago

You can concatenate b4 to b1 first : b41 = ConcatSignal(b4, b3, b2, b1) I think that return (b41 >> (dio +boffset)) & ((1 << width) - 1) will then work fine. As well as return b41(dio + boffset + width, dio + boffset) and return b41[dio + boffset + width : dio + boffset] and perhaps also return concat(b4, b3, b2, b1)[dio + boffset + width : dio + boffset] would be fine too.

tomtor commented 5 years ago

Thanks for the feedback!

b41 = ConcatSignal(b4, b3, b2, b1)                                      
return (b41 >> (dio +boffset)) & ((1 << width) - 1)

gives both in Python 2 and 3:

Can't infer variable type: b41

Same error for option 2 and 3.

The last option gives an exception:

Traceback (most recent call last): File "test_deflate.py", line 542, in tb.convert(initial_values=True) File "/home/tom/.local/lib/python3.6/site-packages/myhdl/_block.py", line 342, in convert return converter(self) File "/home/tom/.local/lib/python3.6/site-packages/myhdl/conversion/_toVerilog.py", line 198, in call _convertGens(genlist, vfile) File "/home/tom/.local/lib/python3.6/site-packages/myhdl/conversion/_toVerilog.py", line 518, in _convertGens v.visit(tree) File "/usr/lib/python3.6/ast.py", line 253, in visit return visitor(node) File "/home/tom/.local/lib/python3.6/site-packages/myhdl/conversion/_toVerilog.py", line 1077, in visit_Module self.visit(stmt) File "/usr/lib/python3.6/ast.py", line 253, in visit return visitor(node) File "/home/tom/.local/lib/python3.6/site-packages/myhdl/conversion/_toVerilog.py", line 1393, in visit_FunctionDef self.visit_stmt(node.body) File "/home/tom/.local/lib/python3.6/site-packages/myhdl/conversion/_toVerilog.py", line 1251, in visit_stmt self.visit(stmt) File "/usr/lib/python3.6/ast.py", line 253, in visit return visitor(node) File "/home/tom/.local/lib/python3.6/site-packages/myhdl/conversion/_toVerilog.py", line 1005, in visit_If self.mapToIf(node) File "/home/tom/.local/lib/python3.6/site-packages/myhdl/conversion/_toVerilog.py", line 1067, in mapToIf self.visitstmt(node.else) File "/home/tom/.local/lib/python3.6/site-packages/myhdl/conversion/_toVerilog.py", line 1251, in visit_stmt self.visit(stmt) File "/usr/lib/python3.6/ast.py", line 253, in visit return visitor(node) File "/home/tom/.local/lib/python3.6/site-packages/myhdl/conversion/_toVerilog.py", line 1003, in visit_If self.mapToCase(node) File "/home/tom/.local/lib/python3.6/site-packages/myhdl/conversion/_toVerilog.py", line 1030, in mapToCase self.visit_stmt(suite) File "/home/tom/.local/lib/python3.6/site-packages/myhdl/conversion/_toVerilog.py", line 1251, in visit_stmt self.visit(stmt) File "/usr/lib/python3.6/ast.py", line 253, in visit return visitor(node) File "/home/tom/.local/lib/python3.6/site-packages/myhdl/conversion/_toVerilog.py", line 1005, in visit_If self.mapToIf(node) File "/home/tom/.local/lib/python3.6/site-packages/myhdl/conversion/_toVerilog.py", line 1067, in mapToIf self.visitstmt(node.else) File "/home/tom/.local/lib/python3.6/site-packages/myhdl/conversion/_toVerilog.py", line 1251, in visit_stmt self.visit(stmt) File "/usr/lib/python3.6/ast.py", line 253, in visit return visitor(node) File "/home/tom/.local/lib/python3.6/site-packages/myhdl/conversion/_toVerilog.py", line 1005, in visit_If self.mapToIf(node) File "/home/tom/.local/lib/python3.6/site-packages/myhdl/conversion/_toVerilog.py", line 1059, in mapToIf self.visit_stmt(suite) File "/home/tom/.local/lib/python3.6/site-packages/myhdl/conversion/_toVerilog.py", line 1251, in visit_stmt self.visit(stmt) File "/usr/lib/python3.6/ast.py", line 253, in visit return visitor(node) File "/home/tom/.local/lib/python3.6/site-packages/myhdl/conversion/_toVerilog.py", line 1005, in visit_If self.mapToIf(node) File "/home/tom/.local/lib/python3.6/site-packages/myhdl/conversion/_toVerilog.py", line 1067, in mapToIf self.visitstmt(node.else) File "/home/tom/.local/lib/python3.6/site-packages/myhdl/conversion/_toVerilog.py", line 1251, in visit_stmt self.visit(stmt) File "/usr/lib/python3.6/ast.py", line 253, in visit return visitor(node) File "/home/tom/.local/lib/python3.6/site-packages/myhdl/conversion/_toVerilog.py", line 1005, in visit_If self.mapToIf(node) File "/home/tom/.local/lib/python3.6/site-packages/myhdl/conversion/_toVerilog.py", line 1056, in mapToIf self.visit(test) File "/usr/lib/python3.6/ast.py", line 253, in visit return visitor(node) File "/home/tom/.local/lib/python3.6/site-packages/myhdl/conversion/_toVerilog.py", line 881, in visit_Call v.visit(node.tree) File "/usr/lib/python3.6/ast.py", line 253, in visit return visitor(node) File "/home/tom/.local/lib/python3.6/site-packages/myhdl/conversion/_toVerilog.py", line 1077, in visit_Module self.visit(stmt) File "/usr/lib/python3.6/ast.py", line 253, in visit return visitor(node) File "/home/tom/.local/lib/python3.6/site-packages/myhdl/conversion/_toVerilog.py", line 1485, in visit_FunctionDef self.visit_stmt(node.body) File "/home/tom/.local/lib/python3.6/site-packages/myhdl/conversion/_toVerilog.py", line 1251, in visit_stmt self.visit(stmt) File "/usr/lib/python3.6/ast.py", line 253, in visit return visitor(node) File "/home/tom/.local/lib/python3.6/site-packages/myhdl/conversion/_toVerilog.py", line 1495, in visit_Return self.visit(node.value) File "/usr/lib/python3.6/ast.py", line 253, in visit return visitor(node) File "/home/tom/.local/lib/python3.6/site-packages/myhdl/conversion/_toVerilog.py", line 1200, in visit_Subscript self.accessIndex(node) File "/home/tom/.local/lib/python3.6/site-packages/myhdl/conversion/_toVerilog.py", line 1243, in accessIndex self.visit(node.slice.value) File "/usr/lib/python3.6/ast.py", line 253, in visit return visitor(node) File "/home/tom/.local/lib/python3.6/site-packages/myhdl/conversion/_toVerilog.py", line 1257, in visit_Tuple assert self.context != None AssertionError Makefile:8: recipe for target 'build' failed

josyb commented 5 years ago

Can you show a bit more of your code? So I have to type less when trying this out myself. I'd also like to see the use case and perhaps there is a better way (read: to generate better Verilog/VHDL code)

tomtor commented 5 years ago

Sure it is in my MyHDL fork:

https://github.com/tomtor/myhdl/tree/oramBRAM/tom

In deflate.py function get4()

josyb commented 5 years ago

It is a bit unfortunate that you put it inside your myhdl fork, as now I can't fork it. Bit I downloaded the complete zip instead. But this actual deflate.py does not convert. I assume you are working on it. Anyway it is a fairly complex piece.

b41 = ConcatSignal(b4, b3, b2, b1)
return (b41 >> (dio +boffset)) & ((1 << width) - 1) gives both in Python 2 and 3:

Can't infer variable type: b41

You have to move the line b41 = ConcatSignal(b4, b3, b2, b1) outside the function to, say right after where you have declared b1,...b4. ConcatSignal actually creates a new Signal which tracks the changes of the given argument Signals.

tomtor commented 5 years ago

Ah thanks!

Yes I am still working on it and I will move the design to a new repo when it is more polished.

Moving ConcatSignal out of the function is elegant. I will try that.

tomtor commented 5 years ago
b1 = Signal(intbv()[8:])
b2 = Signal(intbv()[8:])
b3 = Signal(intbv()[8:]) 
b4 = Signal(intbv()[8:]) 

b41 = ConcatSignal(b4, b3, b2, b1)

gives Icarus warning:

test_fast_bench.v:109: warning: Part select deflate0_b41[31:24] is out of range.
test_fast_bench.v:110: warning: Part select deflate0_b41[23:16] is out of range.
test_fast_bench.v:111: warning: Part select deflate0_b41[15:8] is out of range.
test_fast_bench.v:112: warning: Part select deflate0_b41[7:0] is out of range.

generated verilog:

assign deflate0_b41[32-1:24] = deflate0_b4;
assign deflate0_b41[24-1:16] = deflate0_b3;
assign deflate0_b41[16-1:8] = deflate0_b2;
assign deflate0_b41[8-1:0] = deflate0_b1;

and get4() compiles but does not work...

josyb commented 5 years ago

I took a fresh copy of your work and tried the ConcatSignal myself. MyHDL does not declare the b41 wire in the Verilog code (nor does it declare the b41 signal in the VHDL code -- I don't do Verilog normally ... but I am learning). Very odd, because MyHDL's test-suite passes, and I specifically tested another piece of code to verify that myself. So you have found an obscure bug?

tomtor commented 5 years ago

Could it be a problem that b41 is only used in the function get4() and never outside it?

I have also seen (before trying ConcatSignal)

Signal is driven but not read: b1

which is obviously not true.

josyb commented 5 years ago

That's a valid and helpful remark; b41 is never marked as read and hence silently discarded. This means we have to mark the signal as read when we actually call a function where it is used. Adding b41._markUsed() after b41 = ConcatSignal(b4, b3, b2, b1) outputs the declaration, but we now get an extra warning that the Signal is not read . Adding b41._markRead() will remove that warning (at least for Verilog, but not for VHDL?) but I agree this is far from elegant.

josyb commented 5 years ago

BTW there are other issue with the generated VHDL: the functions should be marked as impure as they use global variables. Also it doesn't like the generated shift_left construct. And MyHDL generates a function instance for every call. Perhaps that's why I rarely use functions in my code ...

josyb commented 5 years ago

@cfelton once spoke about inlining functions; the experiment here makes a point.

tomtor commented 5 years ago

I moved the code to its own repo

https://github.com/tomtor/HDL-deflate