rdaly525 / coreir

BSD 3-Clause "New" or "Revised" License
100 stars 24 forks source link

Add verilog codegen support for wrapped/inline verilog modules #833

Closed leonardt closed 4 years ago

leonardt commented 4 years ago

This allows compiling a top-level module that contains wrapped or inline verilog.

leonardt commented 4 years ago

Fixes error such as

ERROR: Module: global.Monitor
  Type: {'CLK':coreir.clkIn, 'in1':BitIn[4], 'in2':BitIn[4], 'out':BitIn, 'handshake':{'ready':BitIn, 'valid':BitIn}, 'handshake_arr':{'ready':BitIn, 'valid':BitIn}[3], 'mon_temp1':BitIn, 'mon_temp2':BitIn, 'intermediate_tuple':{'_0':BitIn, '_1':BitIn}}
  Def? No has no def!
rsetaluri commented 4 years ago

Just making sure: if there is inline_verilog metadata and a normal CoreIR def, both are compiled right? and inline_verilog is added to the end?

What if both verilog and inilne_verilog are specified?

Otherwise, LGTM.

leonardt commented 4 years ago

Case 1: yes, inline verilog is appended to the end Case 2: it will use the verilog definition and ignore the inline_verilog. We could either raise an error/warning if this occurs, or we could append the inline verilog to the end of the verilog (in one case).

There's two cases when there's verilog metadata. The first, primary use case (for users), is to have an entire string representing the verilog version of the coreir module (this includes the module/port definition, e.g. for wrapped verilog). In this case, we can't append the inline verilog since the string will also include the endmodule statement (so it would be appended afterwards). The second, internal use case (for the verilog primitives) is to have a string just for the body of the module (so coreir generates the verilog module/port list). In this case, it's basically the same as inline_verilog (except inline_verilog supports attaching to the end of a definition, whereas the verilog logic assumes it's the entire definition). Since this is just used internally, I don't think we need to support inline_verilog with verilog, but we could add it. I think it's best to just add a warning if verilog and inline_verilog are used together and say inline_verilog will be ignored