feat(LogPerfHelper): reuse LogPerfHelper if visible #86
Conversation
Modified from Difftest Repo. See details in Difftest PR#261 and #309. To avoid a large number of duplicated LogPerfHelper instantiated in same module, we allow reusing previous instances if visible. Note instances in when scope cannot be accessed from other, such instances is not visible and cannot be reused, we will still instantiate another duplicated one.
|
After this change, number of LogPerfHelper instantiated in XiangShan reduces from thousands to nearly 300, which will speed up compilation speed of Palladium. And other simulator may also benefit from it. |
|
Chisel |
| private val instances = scala.collection.mutable.ListBuffer.empty[LogPerfIO] | ||
| private def instantiate(): LogPerfIO = instances.addOne(WireInit(Module(new LogPerfHelper).io)).last | ||
|
|
||
| def apply(): LogPerfIO = instances.find(gen => isVisible(gen)).getOrElse(instantiate()) |
There was a problem hiding this comment.
One question: after this change, the design will not be compatible with older versions of Chisel before v6.1.0. Is this accpepted by XiangShan and its submodules? @Tang-Haojin
Instead of calling isVisible here, I would prefer using Scala reflection to call the LogPerfControl in DiffTest. Only if that does not exist (the module using utility is not simulated by DiffTest), we use the one currently defined in utility. Then we don't need to copy the isVisible code here and the code would always align with DiffTest. I believe in the future, we will replace the one in DiffTest with tapAndRead.
There was a problem hiding this comment.
One question: after this change, the design will not be compatible with older versions of Chisel before v6.1.0. Is this accpepted by XiangShan and its submodules? @Tang-Haojin
This is okay for XiangShan, but may be carefully checked for submodules.
Instead of calling
isVisiblehere, I would prefer using Scala reflection to call the LogPerfControl in DiffTest. Only if that does not exist (the module using utility is not simulated by DiffTest), we use the one currently defined in utility. Then we don't need to copy the isVisible code here and the code would always align with DiffTest. I believe in the future, we will replace the one in DiffTest withtapAndRead.
I agree to use tapAndRead instead and actually I have tried it few months ago locally. I pushed it to our repositories just now. Maybe @klin02 can help me to review it and if you think it okay, you may also help to rebase it towards master.
Here are the related repos and branches:
https://github.com/OpenXiangShan/XiangShan/tree/tap-and-read
https://github.com/OpenXiangShan/CoupledL2/tree/tap-and-read
https://github.com/OpenXiangShan/HuanCun/tree/tap-and-read
https://github.com/OpenXiangShan/Utility/tree/tap-and-read
There was a problem hiding this comment.
But tapAndRead also has some dedup issues so I write a transform to dedup manually.
There was a problem hiding this comment.
Previously I also try to use tapAndRead for perfIO(Register source first and assign tap to sink somewhere). However, It brings a lot of middle ports like addSource and addSink.
I think that's because chisel use probe ports (can be seen in firrtl module ports) to implement tap. And output probe(child source to parent sink) can be replaced by hierarchical reference like aa.bb.cc, but input probe will still be generated as IOs.
Does current tap supports hidden middle IO？I also agree modified tap is better to logPerf.
There was a problem hiding this comment.
No, chisel still cannot generate non-hierarchical XMR in Verilog, and it generates IO ports. But I think it is okay.
There was a problem hiding this comment.
I can have a try for hierarchical name first. The problem is instance can only be accessed after elaboration.
If it works, maybe we can collect logPerf directly.
There was a problem hiding this comment.
That is ok but printing hierarchical name (%m) may be a new problem.
I see. This is indeed a problem.
There was a problem hiding this comment.
Hierarchical name(for both data or module) can be accessed by xx.pathName after circuit elaboration ( Stage.execute), which means we may need another method called after elaboration, generating macros of hierarchical module name, is it acceptable?
Alternatively, during elaboration, we can get currentModule name up to top module name using recursive DataMirror.getParent. Like SimTop.Mod1.Mod2... instead of SimTop.Inst1.Inst2... But it may be mixed up with multi instances with same modName
There was a problem hiding this comment.
Seems ChiselDB and Constantin already have similar calling interface after elaboration like addToFileRegisters
There was a problem hiding this comment.
Hierarchical name(for both data or module) can be accessed by xx.pathName after circuit elaboration ( Stage.execute), which means we may need another method called after elaboration, generating macros of hierarchical module name, is it acceptable? Alternatively, during elaboration, we can get currentModule name up to top module name using recursive DataMirror.getParent. Like SimTop.Mod1.Mod2... instead of SimTop.Inst1.Inst2... But it may be mixed up with multi instances with same modName
I think it's good. We can add this optional interface to difftest or utility, to be called after elaboration.
Modified from Difftest Repo. See details in Difftest PR#261 and #309.
To avoid a large number of duplicated LogPerfHelper instantiated in same module, we allow reusing previous instances if visible.
Note instances in when scope cannot be accessed from other, such instances is not visible and cannot be reused, we will still instantiate another duplicated one.