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.

Sign up for GitHub

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

Jump to bottom

feat(LogPerfHelper): reuse LogPerfHelper if visible #86

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+26 −18
Open

feat(LogPerfHelper): reuse LogPerfHelper if visible #86

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/main/scala/utility/LogPerfHelper.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package utility

import chisel3._
import chisel3.util.HasBlackBoxInline
import chisel3.reflect.DataMirror.isVisible

class LogPerfIO extends Bundle {
val timer = UInt(64.W)
Expand Down Expand Up @@ -37,3 +38,10 @@ class LogPerfHelper extends BlackBox with HasBlackBoxInline {
|""".stripMargin
setInline("LogPerfHelper.v", verilog)
}

object LogPerfControl {
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())
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 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.

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

Copy link
Member

Choose a reason for hiding this comment

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

But tapAndRead also has some dedup issues so I write a transform to dedup manually.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

No, chisel still cannot generate non-hierarchical XMR in Verilog, and it generates IO ports. But I think it is okay.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

That is ok but printing hierarchical name (%m) may be a new problem.

I see. This is indeed a problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems ChiselDB and Constantin already have similar calling interface after elaboration like addToFileRegisters

Copy link
Member

@poemonsense poemonsense Nov 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

}
2 changes: 1 addition & 1 deletion src/main/scala/utility/LogUtils.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ object XSLog {
val enableDebug = logOpts.enableDebug && debugLevel != XSLogLevel.PERF
val enablePerf = logOpts.enablePerf && debugLevel == XSLogLevel.PERF
if (!logOpts.fpgaPlatform && (enableDebug || enablePerf || debugLevel == XSLogLevel.ERROR)) {
val ctrlInfo = ctrlInfoOpt.getOrElse(Module(new LogPerfHelper).io)
val ctrlInfo = ctrlInfoOpt.getOrElse(LogPerfControl())
val logEnable = ctrlInfo.logEnable
val logTimestamp = ctrlInfo.timer
val check_cond = (if (debugLevel == XSLogLevel.ERROR) true.B else logEnable) && cond
Expand Down
34 changes: 17 additions & 17 deletions src/main/scala/utility/PerfCounterUtils.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,17 +45,17 @@ object XSPerfAccumulate extends HasRegularPerfName {
def apply(perfName: String, perfCnt: UInt)(implicit p: Parameters): Unit = {
judgeName(perfName)
if (p(PerfCounterOptionsKey).enablePerfPrint) {
val helper = Module(new LogPerfHelper)
val perfClean = helper.io.clean
val perfDump = helper.io.dump
val helper = LogPerfControl()
val perfClean = helper.clean
val perfDump = helper.dump

val counter = RegInit(0.U(64.W)).suggestName(perfName + "Counter")
val next_counter = WireInit(0.U(64.W)).suggestName(perfName + "Next")
next_counter := counter + perfCnt
counter := Mux(perfClean, 0.U, next_counter)

when (perfDump) {
XSPerfPrint(p"$perfName, $next_counter\n")(helper.io)
XSPerfPrint(p"$perfName, $next_counter\n")(helper)
}
}
}
Expand All @@ -78,9 +78,9 @@ object XSPerfHistogram extends HasRegularPerfName {
(implicit p: Parameters): Unit = {
judgeName(perfName)
if (p(PerfCounterOptionsKey).enablePerfPrint) {
val helper = Module(new LogPerfHelper)
val perfClean = helper.io.clean
val perfDump = helper.io.dump
val helper = LogPerfControl()
val perfClean = helper.clean
val perfDump = helper.dump

val sum = RegInit(0.U(64.W)).suggestName(perfName + "Sum")
val nSamples = RegInit(0.U(64.W)).suggestName(perfName + "NSamples")
Expand All @@ -103,11 +103,11 @@ object XSPerfHistogram extends HasRegularPerfName {
}

when (perfDump) {
XSPerfPrint(p"${perfName}_sum, ${sum}\n")(helper.io)
XSPerfPrint(p"${perfName}_mean, ${sum/nSamples}\n")(helper.io)
XSPerfPrint(p"${perfName}_sampled, ${nSamples}\n")(helper.io)
XSPerfPrint(p"${perfName}_underflow, ${underflow}\n")(helper.io)
XSPerfPrint(p"${perfName}_overflow, ${overflow}\n")(helper.io)
XSPerfPrint(p"${perfName}_sum, ${sum}\n")(helper)
XSPerfPrint(p"${perfName}_mean, ${sum/nSamples}\n")(helper)
XSPerfPrint(p"${perfName}_sampled, ${nSamples}\n")(helper)
XSPerfPrint(p"${perfName}_underflow, ${underflow}\n")(helper)
XSPerfPrint(p"${perfName}_overflow, ${overflow}\n")(helper)
}

// drop each perfCnt value into a bin
Expand Down Expand Up @@ -142,7 +142,7 @@ object XSPerfHistogram extends HasRegularPerfName {
}

when (perfDump) {
XSPerfPrint(p"${histName}, $counter\n")(helper.io)
XSPerfPrint(p"${histName}, $counter\n")(helper)
}
}
}
Expand All @@ -153,16 +153,16 @@ object XSPerfMax extends HasRegularPerfName {
def apply(perfName: String, perfCnt: UInt, enable: Bool)(implicit p: Parameters): Unit = {
judgeName(perfName)
if (p(PerfCounterOptionsKey).enablePerfPrint) {
val helper = Module(new LogPerfHelper)
val perfClean = helper.io.clean
val perfDump = helper.io.dump
val helper = LogPerfControl()
val perfClean = helper.clean
val perfDump = helper.dump

val max = RegInit(0.U(64.W))
val next_max = Mux(enable && (perfCnt > max), perfCnt, max)
max := Mux(perfClean, 0.U, next_max)

when (perfDump) {
XSPerfPrint(p"${perfName}_max, $next_max\n")(helper.io)
XSPerfPrint(p"${perfName}_max, $next_max\n")(helper)
}
}
}
Expand Down