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

Perf counters #388

Merged
merged 20 commits into from
Mar 26, 2024
Merged

Perf counters #388

merged 20 commits into from
Mar 26, 2024

Conversation

jjjt-git
Copy link

Fixes #361
Not tested, because I did not find the time for it.
So I do not know, whether it works or not.

@Dolu1990
Copy link
Member

Dolu1990 commented Feb 5, 2024

Hi,

One thing would be to preserve backward compatibility, i mean in particualr the mcycleAccess = CsrAccess.NONE, and others.

Idea being to preserve that feature there, while also havig the possibility to have it implemented by your plugin :)

@jjjt-git
Copy link
Author

jjjt-git commented Feb 5, 2024

I added the legacy counters back in, but made sure, they will not be generated if a CounterService is part of the pipeline.
I still have not tested the CounterPlugin, but hope to get to that later that week...

@Dolu1990
Copy link
Member

Dolu1990 commented Feb 6, 2024

Ok thanks :)

@jjjt-git
Copy link
Author

The Plugin works.

@@ -122,12 +122,8 @@ object VexRiscvAhbLite3{
mscratchGen = false,
mcauseAccess = CsrAccess.READ_ONLY,
mbadaddrAccess = CsrAccess.READ_ONLY,
mcycleAccess = CsrAccess.NONE,
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to still keep those option ?
I know that would be redundant to the CounterPlugin implementation, but that would preserve backward compatibility without any disruption. (not break users setups)

Copy link
Member

Choose a reason for hiding this comment

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

(this is a general comment, so would also apply for the other demo)

Copy link
Author

Choose a reason for hiding this comment

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

The options are still available in the CSRPlugin. I removed them from here and other places, since CsrAccess.NONE was and is the default value for these.

Copy link
Member

Choose a reason for hiding this comment

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

Would be possible to revert all the removals of "mcycleAccess = CsrAccess.NONE" and similar things ?

They are there to make it clear to the user that it isn't included, and that if they want they can just turn it on there.

@@ -150,7 +150,7 @@ class VexRiscvSmpClusterWithPeripherals(p : VexRiscvSmpClusterParameter) extends
plic.addTarget(core.cpu.externalSupervisorInterrupt)
List(clint.logic, core.cpu.logic).produce {
for (plugin <- core.cpu.config.plugins) plugin match {
case plugin: CsrPlugin if plugin.utime != null => plugin.utime := clint.logic.io.time
Copy link
Member

Choose a reason for hiding this comment

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

Same here, would need to preserve that feature in the CsrPlugin. (even if that is a duplication) to not break people code

Copy link
Author

Choose a reason for hiding this comment

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

The feature is still in CSRPlugin, even if disabled when CounterService is present (which in this demo it is), so it should break no existing code which uses the Counters in CsrPlugin.

@jjjt-git
Copy link
Author

I reverted the removal of the config options.
The GenFull still uses the CounterPlugin, as does the VexRiscvAxi4LinuxPlicClint.
However there is (currently) no module in the repo to provide performance counters. It should be quite simple to add to the various cache-modules, but I have not looked into it yet.

@Dolu1990 Dolu1990 changed the base branch from master to dev March 22, 2024 08:00
@Dolu1990
Copy link
Member

Thanks ^^
The pr seems all good to me, i just updated it to go into dev instead of master.

Also note since november i'm working on VexiiRiscv :
https://github.com/SpinalHDL/VexiiRiscv

medium / long terms the idea is to migrate toward it.
Let's me know if you are interrested into the project ^^

@@ -649,7 +654,7 @@ class CsrPlugin(val config: CsrPluginConfig) extends Plugin[VexRiscv] with Excep
pipeline.update(MPP, UInt(2 bits))

if(withExternalMhartid) externalMhartId = in UInt(mhartidWidth bits)
if(utimeAccess != CsrAccess.NONE) utime = in UInt(64 bits) setName("utime")
if(pipeline.serviceExist(classOf[CounterService]) && utimeAccess != CsrAccess.NONE) utime = in UInt(64 bits) setName("utime")
Copy link
Member

Choose a reason for hiding this comment

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

Ahhh this seems broken, shouldn't it be !pipeline.serviceExist(classOf[CounterService]) ?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, this must have slipped by.

@jjjt-git
Copy link
Author

Fixed the condition.

@Dolu1990 Dolu1990 merged commit 0e4032c into SpinalHDL:dev Mar 26, 2024
@Dolu1990
Copy link
Member

Thanks ^^

@jjjt-git
Copy link
Author

my pleasure

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.

RiscV Counters
3 participants