zachjs / sv2v

SystemVerilog to Verilog conversion
BSD 3-Clause "New" or "Revised" License
561 stars 55 forks source link

src/sv2v.hs: Remove duplicate source files #144

Closed flaviens closed 3 years ago

flaviens commented 3 years ago

In many projects, the dependency tool usage does not ensure to provide only one single instance of each input file. An example which produces multiple times the same source file is here: https://github.com/openhwgroup/cva6/blob/6000e32b4181304901adc4c8d72870ebc94473a5/Makefile#L147.

This straightforward PR removes duplicate input source files. It makes sv2v more compatible with existing build systems.

zachjs commented 3 years ago

Both Yosys and iverilog error out on when the same file is passed twice because of the duplicate module definitions. Verilator produces a warning instead. I'm guessing this is also true of most commercial tools. I'm hesitant to add this filtering if there is no clear precedent among existing tools. Am I missing something here?

Note that sv2v does not currently check for duplicate modules, although I'd argue it should given the requirements of downstream tools.

flaviens commented 3 years ago

Indeed, the choice of what to do is not trivial. This PR proposes a transparent filtering of duplicate files to enable transparent insertion of a sv2v+Yosys flow before Verilator, while currently this is not possible without removing the duplicates beforehand, since as you mentioned Verilator tolerates duplicates, but Yosys does not.

Maybe instead (or upon) this PR sv2v should warn when duplicates are detected, or maybe a flag could be added to tolerate duplicates for instance. Feel free to modify this proposal how you feel suits the best to sv2v's job! :)

zachjs commented 3 years ago

I believe it would be out of scope for sv2v to filter input files in this way, even if it is hidden behind a flag. Using overlapping globbing patterns seems a niche use case. Verilator's tolerance of duplicate modules does not seem intended to suit this "duplicate files" scenario.

I'm wondering if something like the following would work for you:

printf '%s\n' */*/*.v test/*/*.v test/relong/port_connections_tb.v | awk '!x[$0]++'

You can then pipe that file list into xargs to pass them to some other command. When run in the sv2v repo, this only prints the path to each Verilog file once, despite the overlapping patterns. You could use sort and uniq instead of awk, but the above version keeps the files in their original order, which might be important in some circumstances.

flaviens commented 3 years ago

I understand and accept your position regarding this, and indeed adding one or two commands to the flow is also acceptable.