Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for additional Verilator modes #389

Merged
merged 12 commits into from Nov 21, 2023

Conversation

sifferman
Copy link
Contributor

Hello! I was having troubles getting Verilator XML dumps working with the existing Edalize version, so I added it as a verilator->mode option.

I'm not sure if better documentation should be provided for it, but I didn't want to be too verbose

Thanks!

@sifferman
Copy link
Contributor Author

sifferman commented Aug 7, 2023

Actually, now that I think about it, I should probably just add all the remaining modes:

Existing:

  • --cc
  • --sc
  • --lint-only

New:

  • --binary
  • --dpi-hdr-only
  • --xml-only
  • -E

@sifferman
Copy link
Contributor Author

sifferman commented Aug 7, 2023

Should be all ready for review!

This adds the following modes:

binary (--binary)
dpi-hdr-only (--dpi-hdr-only)
preprocess-only (-E)
xml-only (--xml-only)

I also removed --exe from the .vc file when in the modes "binary", "dpi-hdr-only", "lint-only", "preprocess-only", or "xml-only".

@sifferman sifferman changed the title Added verilator --xml-only support Added support for additional Verilator modes Aug 7, 2023
@sifferman
Copy link
Contributor Author

Actually, I just had a new idea for 3 additional Verilator fields:

  • gen-xml: <bool>
  • gen-dpi-hdr: <bool>
  • xml-output: <filename : String>

This way, using the same run command on a single target, we could generate an XML file, and a DPI.h file.

If gen-xml was enabled, verilator could just be run once more, except with the option --xml-only. If gen-dpi-hdr was enabled, verilator could just be run once more, except with the option --dpi-hdr-only.

This could even potentially replace the dpi-hdr-only and xml-only modes. -E only prints to stdout, so I don't think this deserves its own field.

@olofk
Copy link
Owner

olofk commented Aug 16, 2023

I think this is great, but let's just think it through. As you say, there are different ways to accomplish this, so let's find something that has a good balance between complexity and user friendliness. I haven't really dug into this part of Verilator. Are all these modes mutually exclusive, or could some modes just be added as verilator_options instead?

@sifferman
Copy link
Contributor Author

At least one of the following flags must be set [ref], and the documentation implies them to be mutually exclusive [ref]:

--binary, --cc, --sc, --dpi-hdr-only, --lint-only, --xml-only, -E

Selecting more than one leads to unexpected behavior.

Example

core snippet

      verilator:
        verilator_options:
          - --xml-only

error

make[1]: Vtop.mk: No such file or directory
make[1]: *** No rule to make target 'Vtop.mk'.  Stop.

fixed core snippet

      verilator:
        mode: lint-only
        verilator_options:
          - --xml-only

With FuseSoC, to get XML output, you must specify verilator_options: --xml-only and mode: lint-only, which seems weird. Plus there is no current way to get DPI header output because --lint-only overwrites --dpi-hdr-only, and --dpi-hdr-only + --cc/--sc is not allowed.

Therefore, I feel like creating a mode for each makes a lot of sense.

@sifferman
Copy link
Contributor Author

sifferman commented Sep 2, 2023

Small update:

Wilson just merged my pull request that explicitly enforces the flags (--build, -E, --dpi-hdr-only, --lint-only, --xml-only) to be mutually exclusive (verilator/verilator#4409)

Now the --xml-only example I gave no longer works:

      verilator:
        mode: lint-only
        verilator_options:
          - --xml-only

due to

%Error: The following cannot be used together: --lint-only, --xml-only. Suggest see manual

@sifferman
Copy link
Contributor Author

Alright, @olofk. How does this look?

I expanded the Verilator Makefile to include the modes as callable Makefile targets:

.PHONY: binary dpi-hdr-only lint-only preprocess-only xml-only
binary:
$(EDALIZE_LAUNCHER) $(VERILATOR) --binary -f $(VC_FILE) $(VERILATOR_OPTIONS)
dpi-hdr-only:
$(EDALIZE_LAUNCHER) $(VERILATOR) --dpi-hdr-only -f $(VC_FILE) $(VERILATOR_OPTIONS)
lint-only:
$(EDALIZE_LAUNCHER) $(VERILATOR) --lint-only -f $(VC_FILE) $(VERILATOR_OPTIONS)
preprocess-only V$(TOP_MODULE).i:
$(EDALIZE_LAUNCHER) $(VERILATOR) -E -f $(VC_FILE) $(VERILATOR_OPTIONS) > V$(TOP_MODULE).i
xml-only V$(TOP_MODULE).xml:
$(EDALIZE_LAUNCHER) $(VERILATOR) --xml-only -f $(VC_FILE) $(VERILATOR_OPTIONS)

I also added the following fields gen-xml, gen-dpi-hdr, gen-preprocess, and have them run the corresponding Makefile targets after the usual building due to the specified mode.

{
"name": "gen-xml",
"type": "bool",
"desc": "Generate XML output",
},
{
"name": "gen-dpi-hdr",
"type": "bool",
"desc": "Generate DPI header output",
},
{
"name": "gen-preprocess",
"type": "bool",
"desc": "Generate preprocessor output",
},

# Additional builds
if str(self.tool_options.get("gen-xml")).lower() == "true":
self._run_tool("make", ["xml-only"], quiet=True)
if str(self.tool_options.get("gen-dpi-hdr")).lower() == "true":
self._run_tool("make", ["dpi-hdr-only"], quiet=True)
if str(self.tool_options.get("gen-preprocess")).lower() == "true":
self._run_tool("make", ["preprocess-only"], quiet=True)

I also added an additional mode "none" which when specified, will only do the additional builds specified with gen-xml, gen-dpi-hdr, gen-preprocess

# Build mode
if self.tool_options["mode"] != "none":
self._run_tool("make", args, quiet=True)

sifferman and others added 4 commits September 3, 2023 20:27
from olofk@2a3db66
There seems to be a bug with newer versions of pandas that prevents
kwargs from being passed through the apply function (specifically
passing errors='ignore' to to_numeric in edalize/vivado_reporting.py).
…mming (olofk#392)

Add --pgm flag to tools with programming capabilities
@olofk olofk merged commit cb7af0e into olofk:main Nov 21, 2023
6 of 7 checks passed
@olofk
Copy link
Owner

olofk commented Nov 21, 2023

Took me a while longer than planned to get back to this, but I think it looks all fine now. Thank you for your contributions. Picked and pushed!

@sifferman
Copy link
Contributor Author

Thanks, @olofk!

@sifferman sifferman deleted the verilator-xml-only branch November 22, 2023 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants