Upgrade verilator to support permissive args in the same way as vcs #565
Conversation
It previously only supported them as the last argument. Supporting them in this case would have removed some of the DRY code that is able to handle both simulators.
|
You can now add verilator runtime arguments to |
|
If PERMISSIVE will be required for all simulation targets (VCS, Verilator), does it makes sense to remove the PERMISSIVE and PERMISSIVE_OFF variables defined in vcs/Makefile and verilator/Makefile? |
|
Yeah I think we can hoist them out now. |
| @@ -160,6 +163,8 @@ int main(int argc, char** argv) | |||
| case 'r': rbb_port = atoi(optarg); break; | |||
| case 'V': verbose = true; break; | |||
| case 'D': dramsim = 1; break; | |||
| case 'p': opterr = 0; break; | |||
There was a problem hiding this comment.
why not just call this variable permissive? This isn't telling you that there was an options error right, just masking them?
There was a problem hiding this comment.
I mirrored the variable name in htif/fesvr but I can name it whatever.
There was a problem hiding this comment.
I'll change this after the current CI run finishes in case I need to fix anything else.
There was a problem hiding this comment.
Oh, I see. I mean that's a fine reason to leave it as-is then; do whichever you like better
| @@ -136,6 +136,8 @@ output_dir=$(sim_dir)/output/$(long_name) | |||
| ######################################################################################### | |||
| # helper variables to run binaries | |||
| ######################################################################################### | |||
| PERMISSIVE_ON=+permissive | |||
There was a problem hiding this comment.
Why not just inline this instead of keeping around the two variables?
There was a problem hiding this comment.
This is slightly more DRY in my opinion. It should always be the same in both places and this will ensure that. If we inline it someone might change one or the other without realizing they need to change both.
That my opinion and if others think we should inline I'm ok with that.
Fixes #563
Release Notes
Verilator now supports
+permissiveand now can be passed its runtime arguments such as+verilator+rand+reset+0