Fix Arg.name and earlyLocalName for probes

[tmckay-sifive]

Contributor Checklist

• Did you add Scaladoc to every public function/method?
• Did you add at least one test demonstrating the PR?
• Did you delete any extraneous printlns/debugging code?
• Did you specify the type of improvement?
• Did you add appropriate documentation in docs/src?
• Did you request a desired merge strategy?
• Did you add text to be included in the Release Notes for this change?

Type of Improvement

• Bugfix

Names for probe Args will be the default toString. For example, if you use DataMirror to get the earlyName of a probe (derived from Arg.name), you will see this default toString.

import chisel3._
import chisel3.probe._
import chisel3.reflect.DataMirror
import chisel3.util.experimental.BoringUtils
import circt.stage.ChiselStage

class ProbedClockedData[D <: Data](gen: => D) extends Bundle {
  val dataProbe = Probe(gen)
  def data      = probe.read(dataProbe)

  val clockProbe = Probe(Clock())
  def clock      = probe.read(clockProbe)

  val resetProbe = Probe(Reset())
  def reset      = probe.read(resetProbe)

  def tapAndConnect(ref: D, clock: Clock, reset: Reset) = {
    dataProbe  := BoringUtils.tap(ref)
    clockProbe := BoringUtils.tap(clock)
    resetProbe := BoringUtils.tap(reset)
  }
}

class Dut extends Module {
  val io = IO(new Bundle {
    val in  = Input(UInt(4.W))
    val out = Output(UInt(4.W))
  })
}

class TestHarness extends Module {
  val dut    = Module(new Dut)
  val probed = Wire(new ProbedClockedData(chiselTypeOf(dut.io)))
  probed.tapAndConnect(dut.io, dut.clock, dut.reset)

  println(s"probed.data name: ${DataMirror.queryNameGuess(probed.data)}")
}

object Main extends App {
  println(ChiselStage.emitCHIRRTL(gen = new TestHarness))
}

probed.data name: Node(TestHarness.probed.dataProbe: Wire[AnonymousBundle])

If that name is ever used for an identifier, Chisel will throw an exception due to the invalid name.

Desired Merge Strategy

• Squash: The PR will be squashed and merged (choose this if you have no preference).

Release Notes

Reviewer Checklist (only modified by reviewer)

• Did you add the appropriate labels? (Select the most appropriate one based on the "Type of Improvement")
• Did you mark the proper milestone (Bug fix: 3.6.x, 5.x, or 6.x depending on impact, API modification or big change: 7.0)?
• Did you review?
• Did you check whether all relevant Contributor checkboxes have been checked?
• Did you do one of the following when ready to merge:
  • Squash: You/ the contributor Enable auto-merge (squash), clean up the commit message, and label with Please Merge.
  • Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: tmckay-sifive / name: Trevor McKay (4c3c381, 02f98df, e2a7e01, 0b09421, fe22351)

[mikeurbach]

I think this makes sense, but can you please add a unit test like the example in the PR description that shows the issue and confirms we don't regress in the future?

[dtzSiFive]

Thanks for identifying and chasing this down!!

[jackkoenig]

LGTM but agree it needs a test. Not sure of the best place for this specific test, something in ProbeSpec would be fine or it could be treated as a DataMirror check like here:

chisel/src/test/scala/chiselTests/reflect/DataMirrorSpec.scala

Line 141 in da4cb2d

import DataMirror.queryNameGuess

[tmckay-sifive]

I added a simple unit test. But, there may be more to this. This test

https://github.com/tmckay-sifive/chisel/blob/02f98df3c2c47a087b47ee91b0321a2435cd1727/src/test/scala/chiselTests/ProbeSpec.scala#L370-L389

is failing with

"signalName/pathName should be called after circuit elaboration: ProbeSpec_Anon.w: Wire[Bool], Some(chiselTests.ProbeSpec$$anon$29@6780e0a6)" did not include substring "Cannot define a probe on a non-equivalent type." (ProbeSpec.scala:384)

[jackkoenig]

Lol, looks like that test was expecting the previous bad output, you should just update that test!

[tmckay-sifive]

This test was failing due to missing cases in earlyLocalName for probes. I added cases for them, and this error message seems sensible to me now.

[jackkoenig]

Suggested change

	class TestMod extends RawModule {
	val a = IO(Output(Probe(UInt())))
	val w = WireInit(UInt(32.W), 0.U)
	a := w
	require(reflect.DataMirror.queryNameGuess(a) == "a")
	}
	class TestMod extends RawModule {
	val a = IO(Output(Probe(UInt())))
	val w = WireInit(UInt(32.W), 0.U)
	a := w
	require(reflect.DataMirror.queryNameGuess(a) == "a")
	}
	ChiselStage.emitCHIRRTL(new TestMod)

You have to actually run elaboration for this test to do anything. This is why I always make sure my unit tests fail before they pass (e.g. change the check to say it's equal to "b", see it fail, then fix it).

[jackkoenig]

Suggested change

	case Some(ProbeExpr(Node(ref))) => s"probe(${earlyLocalName(ref, includeRoot)})"
	case Some(RWProbeExpr(Node(ref))) => s"rwprobe(${earlyLocalName(ref, includeRoot)})"
	case Some(ProbeRead(Node(ref))) => s"read(${earlyLocalName(ref, includeRoot)})"
	case Some(ProbeExpr(Node(ref))) => s"${earlyLocalName(ref, includeRoot)}"
	case Some(RWProbeExpr(Node(ref))) => s"${earlyLocalName(ref, includeRoot)}"
	case Some(ProbeRead(Node(ref))) => s"${earlyLocalName(ref, includeRoot)}"

Probe information is not part of the name

[jackkoenig]

Looks great, thanks Trevor!