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

Use PRINTF_FD for rtl event #958

Merged
merged 6 commits into from
Jan 25, 2025
Merged

Use PRINTF_FD for rtl event #958

merged 6 commits into from
Jan 25, 2025

Conversation

FanShupei
Copy link
Contributor

No description provided.

@FanShupei FanShupei requested a review from Avimitin January 24, 2025 09:05
Copy link
Contributor

@Avimitin Avimitin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the design of the passthru.buildCmdArgs in sv-to-vcs/verilator derivation. This design will make it hard to debug the runtime argument, lead to options overriding hell and blurring the boundary between runtime and compile-time.

I can get the idea of separating dpiLib assertion from runEmu derivation. So here are opinions: we could add compile-time metadata to the simulator wrapper, and validate runtime argument using those metadata.

For example:

  1. Move default global arguments to the wrapper
  2. Add a isRuntimeLoad=@isRuntimeLoad@ variable to wrapper, and substitute the value at compile time. related document. Then we can check if user pass sv-lib incorrectly by the arguments the wrapper received.
  3. Since the wrapper becomes complicated now, I suggest remove the current bash implementation embedded in installPhase and replace it with a standalone Python script. This can help us escape from the bash escape hell. And the wrapper installation can be simplifed to substituteAll ${./sim-wrapper.py} $out/${mainProgram}.
  4. move plusargs to upper scope, I think runEmu should only run emulator with given arguments. And those extra stuff should only appear with their requirement. Eg. +t1_wave_path should be placed next to where the *-emu-trace value was specified, but not be automatically generated within runEmu. Also remove the optional installation logic in runEmu. Waveform installation script should also be specified next to the *-trace emulator.

@FanShupei
Copy link
Contributor Author

Hi, may you take a look at the new design.

Move default global arguments to the wrapper

This is addressed in the new emulator.driverWithArgs. It packs the emulator with its default arguments. I feel its simple enough and no bother to instantiate an explicit wrapper.

Add a isRuntimeLoad=@isRuntimeLoad@ variable to wrapper

Now all dpi libs handled in compile time. I find a clever way to avoid nix rebuild for runtime load dpilibs.

So here are opinions: we could add compile-time metadata to the simulator wrapper, and validate runtime argument using those metadata.

I do not have a clear understanding what "simulator wrapper" means here. In the new design, the emulator parameter of runEmu is actually a dict (driverWithArgs and some metadata), but happens to be a derivation (abusing passthru) to make code shorter. Maybe it coincides with your simulator wrapper idea?

move plusargs to upper scope, I think runEmu should only run emulator with given arguments. And those extra stuff should only appear with their requirement. Eg. +t1_wave_path should be placed next to where the *-emu-trace value was specified, but not be automatically generated within runEmu. Also remove the optional installation logic in runEmu.

I'm a bit not in favor of it. Although it's a bit messy, but I feel more important to keep all relevant logics in one central place, including what files should be prepared before run simulator, the exact arguments passed to simulator (though end users may not understand how driverPhase is constructed, its content is observable in log), which artifacts are saved after simulation. I'm a bit afraid hiding some arguments or some actions inside some wrappers may make debugging EDA flow issues more hard.

I think runEmu should only run emulator with given arguments.

Run emulator could be simply done by driverWithArgs ++ plusargs. I think runEmu should focus on simulator-agnostic logic, like constructing plusargs, scheduling online and offline checks, etc. Currently non-trace and trace build are two designed work modes, and vcs-emu-cover is done by some "just works" hacks. The code does look a bit ugly...

@FanShupei FanShupei requested a review from Avimitin January 25, 2025 08:42
@Avimitin Avimitin merged commit e44c918 into master Jan 25, 2025
214 checks passed
@Avimitin Avimitin deleted the update-firtool branch January 25, 2025 09:31
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.

2 participants