API for probing internal signals #3088

debs-sifive · 2023-03-14T21:05:37Z

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 state the API impact?
Did you specify the code generation impact?
Did you request a desired merge strategy?
Did you add text to be included in the Release Notes for this change?

Type of Improvement

backend code generation
new feature/API

API Impact

Creates a new API for probes.

Backend Code Generation Impact

Generates probe-related FIRRTL statements.

Desired Merge Strategy

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

Release Notes

Adds an API for probing internal signals.

Reviewer Checklist (only modified by reviewer)

Did you add the appropriate labels?
Did you mark the proper milestone (Bug fix: 3.5.x or 3.6.x depending on impact, API modification or big change: 5.0.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.

dtzSiFive

This looks great!! Wow! Thanks! 🚀

Added a bunch of comments re:spec and missing bits, don't mean to nit but mostly lots of FYI 👍 .

I'm thinking it's more important to be able to generate the right strings in the valid-design case, not to model details of types / constraints / limitations in Chisel? If the latter is important LMK and I'll take a look pushing on this as have been for CIRCT.

firrtl/src/test/scala/firrtlTests/SerializerSpec.scala

firrtl/src/main/scala/firrtl/ir/IR.scala

firrtl/src/test/scala/firrtlTests/SerializerSpec.scala

sequencer · 2023-03-15T00:51:31Z

Wondering if there is any user-level example of the prove API?

debs-sifive · 2023-03-15T15:57:01Z

@sequencer I am working on the user-facing API! I will add it to this PR before i un-draft it :)

jackkoenig · 2023-03-16T17:53:35Z

Wondering if there is any user-level example of the prove API?

We're doing a bit of co-development of new features, once this works end-to-end, I will make sure we add an integration test that shows this working in a simulation (to the extent that we can with Verilator).

firrtl/src/test/scala/firrtlTests/SerializerSpec.scala

sequencer · 2023-03-17T01:35:44Z

Thanks @debs-sifive and @jackkoenig, we are waiting this feature for a long time!

dtzSiFive · 2023-03-20T22:02:18Z

Probably future work, and might already work but not shown here: you can narrow a reference using sub-index and sub-field, internally to CIRCT it's presently called ref.sub but isn't given an explicitly different name in the FIRRTL spec, so if those will work with the new probe types added here, awesome!

Example: https://github.com/dtzSiFive/reftype-inputs/blob/main/spec/forwarding_refs_upwards_narrow.fir -- given a reference to an aggregate, can select a sub-element and forward that reference up the hierarchy, or read only an element of the aggregate: https://github.com/dtzSiFive/reftype-inputs/blob/main/spec/read_subelement.fir .

firrtl/src/main/scala/firrtl/ir/IR.scala

azidar

LGTM if you delete the extra tpe field in the Expr expressions (unless I'm missing something?)

davidbiancolin · 2023-03-28T00:57:40Z

Is this eventually supposed to support FPGA implementations (say with ILAs?). If so it may be useful to have references to clocks around for Read probes, that may reduce the complexity of deducing which clock the probed value is synchronous to (assuming it is synchronous), or save having to inject clock crossings. Just a thought.

core/src/main/scala/chisel3/experimental/Probe.scala

src/test/scala/chiselTests/experimental/ProbeSpec.scala

seldridge · 2023-04-05T21:25:28Z

@davidbiancolin wrote:

Is this eventually supposed to support FPGA implementations (say with ILAs?). If so it may be useful to have references to clocks around for Read probes, that may reduce the complexity of deducing which clock the probed value is synchronous to (assuming it is synchronous), or save having to inject clock crossings. Just a thought.

This is an open question and I'd like to raise this to have the clock association information captured more directly as opposed to relying on an analysis to determine the association.

As an example, it is now possible to define a clock association directly in Chisel:

class ClockAssociatedData[A <: Data](gen: => A) extends Bundle { 
  val data = gen
  val associatedClock = Probe(Clock())
}

Once you have something like this, it starts to become possible to make it trivial to lookup what clock is associated with something.

src/test/scala/chiselTests/experimental/ProbeSpec.scala

seldridge

LGTM

This looks both very clean and consistent with the existing way that Chisel works. Very, very terse tests. That helps immensely with maintainability.

core/src/main/scala/chisel3/Data.scala

core/src/main/scala/chisel3/internal/firrtl/IR.scala

core/src/main/scala/chisel3/probe/ProbeValue.scala

firrtl/src/main/scala/firrtl/ir/IR.scala

seldridge · 2023-04-26T04:35:02Z

src/test/scala/chiselTests/ProbeSpec.scala

+  // Strip SourceInfos and split into lines
+  private def processChirrtl(chirrtl: String): Array[String] =
+    chirrtl.split('\n').map(line => line.takeWhile(_ != '@').trim())


This would be great as a utility in a PR following this.

seldridge · 2023-04-26T04:36:19Z

src/test/scala/chiselTests/ProbeSpec.scala

+    val chirrtl = ChiselStage.emitCHIRRTL(
+      new RawModule {
+        val a = IO(Output(RWProbe(Bool())))
+
+        val w = WireInit(Bool(), false.B)
+        val w_probe = RWProbeValue(w)
+        define(a, w_probe)
+      },
+      Array("--full-stacktrace")
+    )
+    processChirrtl(chirrtl) should contain("define a = rwprobe(w)")


This is nice and terse! Interestingly, I've never thought to use an anonymous new RawModule like that. Clever.

src/test/scala/chiselTests/ProbeSpec.scala

core/src/main/scala/chisel3/connectable/Connection.scala

seldridge · 2023-04-26T04:42:41Z

src/test/scala/chiselTests/ProbeSpec.scala

+  // TODO writable probes of const type should fail
+
+  // TODO const of probe type should fail? --> put check in ConstSpec
+}


These are some mega-terse tests. 💯

These tests are awesome, thank you/great work!

src/test/scala/chiselTests/ProbeSpec.scala

core/src/main/scala/chisel3/Data.scala

core/src/main/scala/chisel3/probe/Probe.scala

core/src/main/scala/chisel3/Data.scala

core/src/main/scala/chisel3/internal/firrtl/Converter.scala

core/src/main/scala/chisel3/internal/package.scala

core/src/main/scala/chisel3/probe/Probe.scala

adding probes to ir/serializer

1ab8b58

debs-sifive changed the title ~~adding probes to ir/serializer~~ Support and create API for probes Mar 14, 2023

scalafmt

6b9aa53

dtzSiFive reviewed Mar 14, 2023

View reviewed changes

attempt to make spec example more sensical

4a4aa22

jackkoenig reviewed Mar 16, 2023

View reviewed changes

firrtl/src/test/scala/firrtlTests/SerializerSpec.scala Outdated Show resolved Hide resolved

breaking up serializer test

c56375d

azidar reviewed Mar 28, 2023

View reviewed changes

firrtl/src/main/scala/firrtl/ir/IR.scala Outdated Show resolved Hide resolved

azidar marked this pull request as ready for review March 28, 2023 00:17

Merge branch 'main' into probes

b33198b

azidar approved these changes Mar 28, 2023

View reviewed changes

azidar changed the title ~~Support and create API for probes~~ Support and create internal command for probes Mar 28, 2023

debs-sifive added 2 commits April 5, 2023 12:36

Merge branch 'chipsalliance:main' into probes

f1791fa

first draft of user API

c64d9ae

debs-sifive changed the title ~~Support and create internal command for probes~~ Support and create user API for probes Apr 5, 2023

debs-sifive requested review from jackkoenig, azidar and dtzSiFive April 5, 2023 21:04

fixing probespec checks

2161ae0

azidar reviewed Apr 5, 2023

View reviewed changes

core/src/main/scala/chisel3/experimental/Probe.scala Outdated Show resolved Hide resolved

azidar reviewed Apr 5, 2023

View reviewed changes

src/test/scala/chiselTests/experimental/ProbeSpec.scala Outdated Show resolved Hide resolved

adding test for connectors

df04b4f

debs-sifive commented Apr 5, 2023

View reviewed changes

src/test/scala/chiselTests/experimental/ProbeSpec.scala Outdated Show resolved Hide resolved

debs-sifive added 12 commits April 14, 2023 15:23

change api to RW/Probe() and RW/ProbeValue() object

1576292

Merge branch 'main' of github.com:chipsalliance/chisel3 into probes

f82c6e7

fixing probe modifier checks for wire/reg/mem

f328002

added macros, make probe of probe illegal

d7ae759

moving force/release methods into RWProbe only

51d599e

create probe package

a03531a

fix converter merge

522708e

support accesses to probe subfields

f590e1f

test case for define type checking

1ba978f

forbid probe of aggregate containing probes

733b5d9

fixing const extractType

f4367af

handling connections for probes

0766aff

seldridge approved these changes Apr 26, 2023

View reviewed changes

seldridge reviewed Apr 26, 2023

View reviewed changes

src/test/scala/chiselTests/ProbeSpec.scala Show resolved Hide resolved

seldridge mentioned this pull request Apr 26, 2023

Handle BoringUtils in Chisel, rather than in FIRRTL compiler #3187

Merged

14 tasks

debs-sifive added 3 commits April 26, 2023 09:38

address schuyler's review

939d1bf

additional tests and checks

59f0ac3

add sourceInfo to Const

957585b