google / xls

XLS: Accelerated HW Synthesis
http://google.github.io/xls/
Apache License 2.0
1.2k stars 177 forks source link

simulators should have an option to support SystemVerilog #553

Open proppy opened 2 years ago

proppy commented 2 years ago

It seems that the iverilog invocation doesn't allow for SystemVerilog support: https://github.com/google/xls/blob/c330e64365e56439ab9496159aa8664c6cd5eb6a/xls/simulation/simulators/iverilog_simulator.cc#L42 Which could cause simulation to fails with verilog generated by codegen_main by default (since use_system_verilog default to true): https://github.com/google/xls/blob/6d593b81ff45ae6fdf15d2fea68bdd5da253200b/xls/tools/codegen_main.cc#L109

/tmp/xls_tempfile_Xzkpj6:34: syntax error
/tmp/xls_tempfile_Xzkpj6:34: error: Invalid module instantiation
/tmp/xls_tempfile_Xzkpj6:36: error: Invalid module instantiation
/tmp/xls_tempfile_Xzkpj6:38: error: invalid module item.

It would be nice if a similar option was available on the VerilogSimulator interface (also defaulting to true!); according to iverilog documentation system verilog can be enabled with the following -g<generation flag> option: https://iverilog.fandom.com/wiki/Iverilog_Flags#-g.3Cgeneration_flag.3E

meheff commented 2 years ago

I think for simulation using commercial tools which we use internally at Google we just unconditionally set it to SystemVerilog mode as SV is (roughly?) a superset of Verilog. We might be able to do the same with iverilog assuming it can consume our SystemVerilog. Does hacking in the -g2012 flag in make it work for your design in SystemVerilog?

I think the latest version (11.0) is required for reasonable SystemVerilog support but we pull in 10.3. If we update to 11.0 we can test iverilog using SystemVerilog with ~all of our tests by adding an entry here: https://github.com/google/xls/blob/6d5be31eedc943072e71079d5aba418e38b12e6f/xls/simulation/simulation_targets.inc#L21

Specifically:

    SimulationTarget{"iverilog",
                     /*use_system_verilog=*/true}

(note the true for use_system_verilog) And changing the iverilog invocation to include -g2012. If that passes then it's very likely that iverilog can handle any SystemVerilog we produce.

meheff commented 2 years ago

Looks like we pull in iverilog 10.3 through bazel_rules_hdl so they'd need to update to 11.0 to test this.

cdleary commented 2 years ago

cc @QuantamHD who I think said he could look into bumping iverilog in bazel_rules_hdl