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.

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

ProbeBase.apply() should error if trying to create a Probe of a non-passive type #3609

Open
debs-sifive opened this issue Oct 30, 2023 · 1 comment
Assignees

Comments

@debs-sifive
Copy link
Contributor

Type of issue: Feature Request

Is your feature request related to a problem? Please describe.
Probes of non-passive types, e.g. bidirectional Bundles are not supported by Chisel/firtool, so it makes sense for Chisel to throw an error instead of waiting for firtool to throw one.

For example

class SimpleBundle() extends Bundle {
  val x = Flipped(Bool())
}
val a = Wire(new SimpleBundle())
val b = IO(Output(RWProbe(new SimpleBundle())))
define(b, RWProbeValue(a))

throws

[info] ------------------------------------------------------------------------------
[info] ExitCode:
[info] 1
[info] STDOUT:
[info] 
[info] STDERR:
[info] <unknown>:0: warning: option -dedup is deprecated since firtool 1.57.0, has no effect (deduplication is always enabled), and will be removed in firtool 1.58.0
[info] src/test/scala/chiselTests/ProbeSpec.scala:379:21: error: unable to lower due to symbol "sym" with target not preserved by lowering
[info]         val a = Wire(new SimpleBundle())
[info]                     ^
[info] 
[info] ------------------------------------------------------------------------------

in firtool, when we could catch this earlier at the Chisel level.

Describe the solution you'd like
When creating probes, i.e. in ProbeBase.apply(), also check that the Data passed in is passive.

protected def apply[T <: Data](source: => T, writable: Boolean)(implicit sourceInfo: SourceInfo): T = {

Describe alternatives you've considered
I tried writing a check using collectFlippedDeep, but that function requires Connectable types (so actual hardware, and not Probes).

if (collectFlippedDeep(data) { _ => true }.nonEmpty) {
  Builder.error("Cannot probe a non-passive type.")
}

gives

[info]   chisel3.package$ExpectedHardwareException: Can only created Connectable of components, not unbound Chisel types 'Bool' must be hardware, not a bare Chisel type. Perhaps you forgot to wrap it in Wire(_) or IO(_)?
[info]   at chisel3.experimental.package$requireIsHardware$.apply(package.scala:66)
[info]   at chisel3.connectable.Connectable.<init>(Connectable.scala:22)
[info]   at chisel3.connectable.Connectable$.apply(Connectable.scala:156)
[info]   at chisel3.Data$.toConnectableDefault(Data.scala:807)
[info]   at chisel3.reflect.DataMirror$.collectFlippedDeep(DataMirror.scala:319)

Additional context
N/A

What is the use case for implementing this feature?
If users try to create RWProbes on types that firtool does not support, they will get an error at Chisel time rather than firtool time.

@dtzSiFive dtzSiFive self-assigned this Oct 31, 2023
@dtzSiFive
Copy link
Member

This should be resolved with a trivial change after #3612 (with dedicated helper for general use doing same as code here does, in #3613).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants