ADD: Timing-Accurate Core Instruction Tracing #3706

iansseijelly · 2025-01-15T19:25:26Z

Related issue:

Type of change: feature request

Impact: API addition (no impact on existing code)

Development Phase: implementation

Release Notes

Added feature for:

rocket core to drive the trace ingress bundle, which is a standard interface for different trace encoder standards.
trace encoder builder in rockettile. This allows the building of trace encoders within the tile, whether it is N-trace, E-trace, or custom.
trace sink builder in rockettile. This allows the building of a sequence of trace sinks. Also supports monitoring the data sinking into the arbiter to cross-check the encoding.
arbiter and controller instantiation in rockettile. This instantiates an arbiter that chooses the different trace sinks, and a trace controller that instantiates the necessary MMIO register routings to control encoders.

linux-foundation-easycla · 2025-01-15T19:25:31Z

The committers listed above are authorized under a signed CLA.

✅ login: iansseijelly / name: Lux Zhang (c25af3e, 754bf4c, 84cbd51, 7667eae, 7ae0c21, 4147a0f, b814ad1, d2bddb8, 8d29be4, 7acebde, 8db9f45, f294695, 24cf6fa, 96f9850, ab79600)

src/main/scala/util/TraceCoreIngressGen.scala

src/main/scala/util/TraceCoreInterface.scala

src/main/scala/util/TraceCoreIngressGen.scala

src/main/scala/rocket/RocketCore.scala

src/main/scala/subsystem/HasTiles.scala

src/main/scala/util/TraceEncoderController.scala

src/main/scala/util/TraceSink.scala

jerryz123 · 2025-01-16T20:38:55Z

src/main/scala/util/TraceSink.scala

+  })
+}
+
+class TraceSinkPrint(bb_name: String)(implicit p: Parameters) extends LazyModule {


What is bb_name?

src/main/scala/util/TraceSink.scala

src/main/scala/util/Blockable.scala

jerryz123 · 2025-01-22T17:56:24Z

src/main/resources/vsrc/TraceSinkMonitor.v

+    input[7:0] in_byte
+);
+
+`ifdef VCS


This should work in all RTL simulators. I think ifndef SYNTHESIS is what you want

jerryz123 · 2025-01-22T18:07:10Z

src/main/scala/trace/TraceSinkAlways.scala

+      val trace_in = Flipped(Decoupled(UInt(8.W)))
+    })
+    withClockAndReset(clock, reset) {
+      io.trace_in.ready := true.B


You don't need withClockAndReset here... there is no synchronous logic

jerryz123 · 2025-01-22T18:08:55Z

src/main/scala/trace/TraceSinkAlways.scala

+import org.chipsalliance.cde.config.{Parameters, Config, Field}
+import freechips.rocketchip.tile._
+import freechips.rocketchip.subsystem._
+case object TraceSinkAlwaysKey extends Field[Option[Int]](None)


What does this Int indicate?

I think this can be removed now

jerryz123 · 2025-01-23T21:53:36Z

src/main/scala/tile/RocketTile.scala

+    trace_encoder_controller
+  }
+
+  val (trace_sinks, trace_sink_ids) = rocketParams.ltrace match {


Suggested change

val (trace_sinks, trace_sink_ids) = rocketParams.ltrace match {

val (trace_sinks, traceSinkIds) = rocketParams.ltrace match {

Informal convention is snake_case for HW vars, camelCase for SW vars

jerryz123 · 2025-01-23T21:54:09Z

src/main/scala/tile/RocketTile.scala

+  }
+
+  val (trace_sinks, trace_sink_ids) = rocketParams.ltrace match {
+    case Some(t) => 


I

Suggested change

case Some(t) =>

case Some(t) => t.buildSinks.map {(p)}.unzip

Does this work?

jerryz123 · 2025-01-23T21:54:28Z

src/main/scala/tile/RocketTile.scala

+    case Some(t) => 
+      val sequence = t.buildSinks.map {_(p)}
+      (sequence.map(_._1), sequence.map(_._2))
+    case None => (Seq.empty, Seq.empty)


Suggested change

case None => (Seq.empty, Seq.empty)

case None => (Nil, Nil)

jerryz123 · 2025-01-23T21:55:51Z

src/main/scala/trace/TraceEncoder.scala

+  bufferDepth: Int,
+  encoderBaseAddr: BigInt,
+  useArbiterMonitor: Boolean,
+  // a seq of functions that takes a parameter and returns a lazymodule and a target id


Can you explain what the target id is for in a comment? I thnk that is not obvious

jerryz123 · 2025-01-23T21:57:01Z

src/main/scala/trace/TraceEncoder.scala

+  val MAX_DELTA_TIME_COMP = 0xCF // 63, 6 bits
+
+  when (io.stall) {
+    printf("TraceEncoder stall detected\n")


This printf should probably not be here

I think this RTL printf is somewhat important, so in simulation we can kind of know when stalls happen. I would prefer to keep it.

It messes up the commit log, and I think there's a compelling reason to include the trace stuff by default in some designs.

You can add a PlusArg flag - PlusArg("trace_debug"), for example, which can generate a HW bool that you can use to gate the printf.

Then at run-time, to enable the TraceEncode prints, you just pass (in CY) EXTRA_SIM_FLAGS="+trace_debug=1"

jerryz123 · 2025-01-23T21:57:59Z

src/main/scala/trace/TraceSinkArbiter.scala

+import freechips.rocketchip.regmapper.{RegField, RegFieldDesc}
+
+class TraceSinkArbiter(nSeq : Seq[Int], use_monitor: Boolean = false, monitor_name: String = "unknown")(implicit p: Parameters) extends LazyModule {
+  println(s"TraceSinkArbiter nSeq: $nSeq")


Prints should be removed.

jerryz123

High level request, can you add in a comment at the beginning of the files you added pointing out what spec exactly you implemented? Like "This is an implementation of the TraceXXX as defined in the RISC-V Trace-XXX spec"

jerryz123 · 2025-01-23T22:03:59Z

src/main/scala/trace/TraceEncoder.scala

+  val MAX_DELTA_TIME_COMP = 0xCF // 63, 6 bits
+
+  when (io.stall) {
+    printf("TraceEncoder stall detected\n")


It messes up the commit log, and I think there's a compelling reason to include the trace stuff by default in some designs.

You can add a PlusArg flag - PlusArg("trace_debug"), for example, which can generate a HW bool that you can use to gate the printf.

Then at run-time, to enable the TraceEncode prints, you just pass (in CY) EXTRA_SIM_FLAGS="+trace_debug=1"

jerryz123 · 2025-01-23T22:05:50Z

src/main/scala/trace/TraceSinkDMA.scala

+import freechips.rocketchip.regmapper.{RegField, RegFieldDesc}
+import freechips.rocketchip.tile._
+
+case object TraceSinkDMAKey extends Field[Option[Int]](None)


We are not putting this in RC, right?

jerryz123 · 2025-01-23T22:07:08Z

src/main/scala/trace/TraceSinkArbiter.scala

+    }
+
+    if (use_monitor) {
+      val monitor = Module(new TraceSinkMonitor(s"trace_monitor_$monitor_name.out"))


Should monitor_name reflect the hartId of the core that is being traced? In a multicore, each core should generate its own trace file, right?

jerryz123 · 2025-01-27T23:38:06Z

src/main/scala/tile/RocketTile.scala

+    case None => (Nil, Nil)
+  }
+
+  val trace_sink_arbiter = rocketParams.traceParams.map { t =>


Can you just merge the arbiter and encoder modules?

jerryz123

I'll pass through this again once you remove all the things that will not be upstreamed @iansseijelly

jerryz123 · 2025-01-27T23:38:43Z

src/main/scala/tile/RocketTile.scala

+    trace_encoder_controller
+  }
+
+  val trace_encoder = rocketParams.traceParams match {


Suggested change

val trace_encoder = rocketParams.traceParams match {

val trace_encoder = rocketParams.traceParams.map(_.buildEncoder(p))

jerryz123 · 2025-01-27T23:39:01Z

src/main/scala/tile/RocketTile.scala

@@ -177,7 +223,7 @@ class RocketTileModuleImp(outer: RocketTile) extends BaseTileModuleImp(outer)

  // Pass through various external constants and reports that were bundle-bridged into the tile
  outer.traceSourceNode.bundle <> core.io.trace
-  core.io.traceStall := outer.traceAuxSinkNode.bundle.stall
+  // core.io.traceStall := outer.traceAuxSinkNode.bundle.stall


@iansseijelly

jerryz123 · 2025-01-27T23:39:45Z

src/main/scala/trace/TacitEncoder.scala

+import chisel3.util._
+import scala.math.min
+
+import org.chipsalliance.cde.config.Parameters


This should be removed from RC then? The abstract encoder is in RC, but the custom encoder should be elsewhere?

jerryz123 · 2025-01-27T23:40:49Z

src/main/scala/trace/TraceSinkMonitor.scala

+
+class TraceSinkMonitor(name: String) extends BlackBox(
+  Map(
+    "FILE_NAME" -> StringParam(s"tacit_${name}_trace.out")


Clean tacit from the naming

iansseijelly · 2025-01-28T00:49:14Z

Hi, I have removed the non-standard components, and addressed the above items.

jerryz123

Only one minor request, TraceSinkArbiter can be a Module, not a LazyModule

jerryz123 · 2025-01-28T18:29:57Z

src/main/scala/trace/TraceSinkArbiter.scala

+import freechips.rocketchip.tile._
+import freechips.rocketchip.regmapper.{RegField, RegFieldDesc}
+
+class TraceSinkArbiter(nSeq : Seq[Int], use_monitor: Boolean = false, monitor_name: String = "unknown")(implicit p: Parameters) extends LazyModule {


This should just be a Module, not a LazyModule

jerryz123 · 2025-01-29T18:57:22Z

src/main/scala/tile/RocketTile.scala

+    outer.trace_encoder_controller.foreach { lm =>
+      outer.trace_encoder.get.module.io.control <> lm.module.io.control
+    }
+


Don't construct the arbiter outside this if block... just do it inside.... then you don't need a Option[Arbiter]

iansseijelly added 3 commits January 15, 2025 09:47

ADD: core insn trace ingress generation

7ae0c21

ADD: tacit trace encoder, controller, and sink

4147a0f

ADD: trace tile instantiation and connection

754bf4c

iansseijelly changed the title ~~Tacit pr~~ ADD: Timing-Accurate Core Instruction Tracing Jan 15, 2025

iansseijelly mentioned this pull request Jan 15, 2025

ADD: config framents and example config for tacit ucb-bar/chipyard#2173

Closed

16 tasks

jerryz123 requested changes Jan 16, 2025

View reviewed changes

iansseijelly added 2 commits January 16, 2025 15:49

ADD: address all PR comments

c25af3e

ADD: Print BlackBox

7667eae

jerryz123 reviewed Jan 18, 2025

View reviewed changes

src/main/scala/util/Blockable.scala Show resolved Hide resolved

iansseijelly added 3 commits January 20, 2025 13:14

REFACTOR: use canhave + traits for sinks for scalability

b814ad1

ADD: bump TraceSinkDMA to inject node from trait instead of in tiles

8d29be4

ADD: tail case bug fix

84cbd51

jerryz123 reviewed Jan 22, 2025

View reviewed changes

REFACTOR: reorganize code structure

ab79600

jerryz123 reviewed Jan 23, 2025

View reviewed changes

ADD: remove prints, cleanup code

7acebde

jerryz123 requested changes Jan 23, 2025

View reviewed changes

iansseijelly added 2 commits January 23, 2025 18:49

ADD: refactor

24cf6fa

FIX: VCS->NOT SYN

f294695

jerryz123 reviewed Jan 27, 2025

View reviewed changes

jerryz123 requested changes Jan 27, 2025

View reviewed changes

RM: non-standard components

96f9850

jerryz123 requested changes Jan 28, 2025

View reviewed changes

FIX: make arbiter not lazy

8db9f45

jerryz123 approved these changes Jan 28, 2025

View reviewed changes

jerryz123 reviewed Jan 29, 2025

View reviewed changes

FIX: move arbiter into inner scope

d2bddb8

jerryz123 merged commit 0b37602 into chipsalliance:master Jan 29, 2025
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ADD: Timing-Accurate Core Instruction Tracing #3706

ADD: Timing-Accurate Core Instruction Tracing #3706

iansseijelly commented Jan 15, 2025 •

edited

Loading

linux-foundation-easycla bot commented Jan 15, 2025 •

edited

Loading

jerryz123 Jan 16, 2025

jerryz123 Jan 22, 2025

jerryz123 Jan 22, 2025

jerryz123 Jan 22, 2025

jerryz123 Jan 23, 2025

jerryz123 Jan 23, 2025

jerryz123 Jan 23, 2025

jerryz123 Jan 23, 2025

jerryz123 Jan 23, 2025

jerryz123 Jan 23, 2025

iansseijelly Jan 23, 2025

jerryz123 Jan 23, 2025

jerryz123 Jan 23, 2025

jerryz123 left a comment

jerryz123 Jan 23, 2025

jerryz123 Jan 23, 2025

jerryz123 Jan 23, 2025

jerryz123 Jan 27, 2025

jerryz123 left a comment

jerryz123 Jan 27, 2025

jerryz123 Jan 27, 2025

jerryz123 Jan 27, 2025

jerryz123 Jan 27, 2025

iansseijelly commented Jan 28, 2025

jerryz123 left a comment

jerryz123 Jan 28, 2025

jerryz123 Jan 29, 2025

	val (trace_sinks, trace_sink_ids) = rocketParams.ltrace match {
	val (trace_sinks, traceSinkIds) = rocketParams.ltrace match {

	val trace_encoder = rocketParams.traceParams match {
	val trace_encoder = rocketParams.traceParams.map(_.buildEncoder(p))

ADD: Timing-Accurate Core Instruction Tracing #3706

ADD: Timing-Accurate Core Instruction Tracing #3706

Conversation

iansseijelly commented Jan 15, 2025 • edited Loading

linux-foundation-easycla bot commented Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryz123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryz123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iansseijelly commented Jan 28, 2025

jerryz123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iansseijelly commented Jan 15, 2025 •

edited

Loading

linux-foundation-easycla bot commented Jan 15, 2025 •

edited

Loading