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

[core] Add Placeholder API #4628

Merged
merged 2 commits into from
Jan 24, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
45 changes: 45 additions & 0 deletions core/src/main/scala/chisel3/Placeholder.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// SPDX-License-Identifier: Apache-2.0

package chisel3

import chisel3.experimental.SourceInfo
import chisel3.internal.Builder
import chisel3.internal.firrtl.ir

/** A [[Placeholder]] is an _advanced_ API for Chisel generators to generate
* additional hardware at a different point in the design.
*
* For example, this can be used to add a wire _before_ another wire:
* {{{
* val placeholder = new Placeholder()
* val a = Wire(Bool())
* val b = placeholder.append {
* Wire(Bool())
* }
* }}}
*
* This will generate the following FIRRTL where `b` is declared _before_ `a`:
* {{{
* wire b : UInt<1>
* wire a : UInt<1>
* }}}
*/
private[chisel3] class Placeholder()(implicit sourceInfo: SourceInfo) {
Copy link
Member

Choose a reason for hiding this comment

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

Should this document that footgun this enables regarding adding uses of hardware before they exist?

I know it says "advanced" but maybe better to be explicit....?

EDIT: There are many footguns possible here (see below), not sure reasonable to document neatly.


private val state = Builder.State.save

private val placeholder = Builder.pushCommand(new ir.Placeholder(sourceInfo = sourceInfo))

/** Generate the hardware of the `thunk` and append the result at the point
* where this [[Placeholder]] was created. The return value will be what the
* `thunk` returns which enables getting a reference to the generated
* hardware.
*
* @param thunk the hardware to generate and append to the [[Placeholder]]
* @return the return value of the `thunk`
*/
def append[A](thunk: => A): A = Builder.State.guard(state) {
Builder.currentBlock.get.appendToPlaceholder(placeholder)(thunk)
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean currentBlock? Probably the expected behavior is that hardware generated in the placeholder belongs to the block containing the placeholder, not the current block we're appending from...?
(offhand not sure where to grab the right block, or I'd offer a concrete suggestion here)

Also, things like layerStack and whenStack will be wrong when evaluating thunk, probably other context bits maintained during top-down evaluation and not captured in the Command list s.t. you can jump around and insert this way.

For the when/layer stacks, these ideally would be replaced with walking up blocks to their parents (this was discussed/planned but postponed for reasons I don't have paged in). Details like elideLayerBlocks are not captured, just tracked as evaluation context. I haven't audited them all to separate those that maybe make sense as context regardless of insertion point, but offhand probably none of them (unless they'd always the same regardless...)?

That is, we're still mostly reliant on "context" state.

To help with the stacks, could require this is invoked from same block as that containing the placeholder -- but that doesn't address the other context details.

Basically, we don't have the infrastructure to do this safely.

What we have barely allows us to insert "secret connections".

Since it's now "private" (thanks, I suggested this as well...) and is probably fine for the use case envisioned for it, if we want to proceed anyway I won't stand in the way. I'd prefer we sorted this all out instead.

Copy link
Member

Choose a reason for hiding this comment

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

Consider this test you can add to PlaceholderSpec.scala:

  they should "do something fun when used across modules" in {

    object A extends layer.Layer(layer.LayerConfig.Extract())

    class Bar extends RawModule {
      val placeholder = new Placeholder()
    }
    class Foo extends RawModule {
      val in = IO(Input(Bool()))

      val bar = Module(new Bar)
      val x = bar.placeholder.append({ val w = Wire(UInt(1.W)); w := 0.U; w })
      println(x)
      x := 1.U
    }

    println(circt.stage.ChiselStage.emitCHIRRTL(new Foo))
    generateFirrtlAndFileCheck(new Foo) {
      s"""|CHECK:      wire
          |CHECK-NEXT: asdf
          |""".stripMargin
    }

  }

Which generates a connection to x from a different module. This is because of the issue mentioned above where the wrong block is used.

Output from print:

[372] circuit Foo :
[372]   layer Verification, bind, "verification" :
[372]     layer Assert, bind, "verification/assert" :
[372]     layer Assume, bind, "verification/assume" :
[372]     layer Cover, bind, "verification/cover" :
[372]   module Bar : @[src/test/scala/chisel3/PlaceholderSpec.scala 108:11]
[372]
[372]     wire x : UInt<1> @[src/test/scala/chisel3/PlaceholderSpec.scala 115:52]
[372]     connect x, UInt<1>(0h0) @[src/test/scala/chisel3/PlaceholderSpec.scala 115:67]
[372]
[372]   public module Foo : @[src/test/scala/chisel3/PlaceholderSpec.scala 111:11]
[372]     input in : UInt<1> @[src/test/scala/chisel3/PlaceholderSpec.scala 112:18]
[372]
[372]     inst bar of Bar @[src/test/scala/chisel3/PlaceholderSpec.scala 114:23]
[372]     connect x, UInt<1>(0h1) @[src/test/scala/chisel3/PlaceholderSpec.scala 117:9]

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this is a problem. Here's what's changed to address this:

  1. 4dd8f19 adds the ability to save and restore the Builder's state. This is a bit of a misnomer as it is really "state related to blocks, modules, layers, and whens", i.e., enough to do things like change the append point. You don't want to actually save the entire builder state as this includes global things.
  2. The Placeholder.append method is updated to save/change/restore the state using a method called guard that takes a thunk. This should then make this approach safe.

I added two tests that show that this does actually work (allows you to construct hardware in the parent) and will error when used improperly (disallows construction of hardware in a child, i.e., a closed module). I do think that it is reasonable to relax the closed module restriction (as this is essentially a pathway to enable a cleaner BoringUtils as well as a Chisel-native provider API).

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks reasonable enough to me, my approval remains

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'm looking at this specific code again. I may merge this and then loop back to change it.

The problem right now is that the Builder expect that it is always working with a Block when, after this patch, it could be inserting into a Block or a Placeholder. I think we probably want this to revert to always pointing at a command builder directly.

}

}
79 changes: 79 additions & 0 deletions core/src/main/scala/chisel3/internal/Builder.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1199,6 +1199,85 @@ private[chisel3] object Builder extends LazyLogging {
}

initializeSingletons()

/** The representation of the state of the [[Builder]] at a current point in
* time. This is intended to capture _enough_ information to insert hardware
* at another point in the circuit.
*
* @see [[chisel3.Placeholder]]
*/
case class State(
currentModule: Option[BaseModule],
whenStack: List[WhenContext],
blockStack: List[Block],
layerStack: List[layer.Layer],
prefix: Prefix,
clock: Option[Delayed[Clock]],
reset: Option[Delayed[Reset]],
enabledLayers: List[layer.Layer]
)

object State {

/** Return a [[State]] suitable for doing module construction.
*/
def default: State = State(
currentModule = Builder.currentModule,
whenStack = Nil,
blockStack = Builder.blockStack,
layerStack = layer.Layer.Root :: Nil,
prefix = Nil,
clock = None,
reset = None,
enabledLayers = Nil
)

/** Capture the current [[Builder]] state.
*/
def save: State = {
State(
currentModule = Builder.currentModule,
whenStack = Builder.whenStack,
blockStack = Builder.blockStack,
layerStack = Builder.layerStack,
prefix = Builder.getPrefix,
clock = Builder.currentClockDelayed,
reset = Builder.currentResetDelayed,
enabledLayers = Builder.enabledLayers.toList
)
}

/** Set the [[Builder]] state to that provided by the parameter.
*
* @param state the state to set the [[Builder]] to
*/
def restore(state: State) = {
Builder.currentModule = state.currentModule
Builder.whenStack = state.whenStack
Builder.blockStack = state.blockStack
Builder.layerStack = state.layerStack
Builder.setPrefix(state.prefix)
Builder.currentClock = state.clock
Builder.currentReset = state.reset
Builder.enabledLayers = enabledLayers
}

/** Run the `thunk` with the context provided by `state`. Save the [[Builder]]
* state before the thunk and restore it afterwards.
*
* @param state change the [[Builder]] to this state when running the thunk
* @param thunk some hardware to generate
* @return whatever the `thunk` returns
*/
def guard[A](state: State)(thunk: => A): A = {
val old = save
restore(state)
val result = thunk
restore(old)
result
}

Comment on lines +1272 to +1279
Copy link
Contributor

Choose a reason for hiding this comment

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

Can/Should this be used in other places? Like Module.apply?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, probably. This is also highlighting that there is more that should be saved here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Much more is now saved with: 50805af

A follow-on PR showing this being used for Module.apply is here: #4641

I expect that this can also be used to replace the related Definition.apply. I haven't looked into this, yet.

}
}

/** Allows public access to the naming stack in Builder / DynamicContext, and handles invocations
Expand Down
4 changes: 3 additions & 1 deletion core/src/main/scala/chisel3/internal/firrtl/Converter.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

package chisel3.internal.firrtl

import chisel3._
import chisel3.{Placeholder => _, _}
import chisel3.experimental._
import chisel3.experimental.{NoSourceInfo, SourceInfo, SourceLine, UnlocatableSourceInfo}
import chisel3.properties.Property
Expand Down Expand Up @@ -266,6 +266,8 @@ private[chisel3] object Converter {
)
case LayerBlock(info, layer, region) =>
fir.LayerBlock(convert(info), layer, convert(region, ctx, typeAliases))
case Placeholder(info, block) =>
dtzSiFive marked this conversation as resolved.
Show resolved Hide resolved
convert(block, ctx, typeAliases)
}

/** Convert Chisel IR Commands into FIRRTL Statements
Expand Down
41 changes: 41 additions & 0 deletions core/src/main/scala/chisel3/internal/firrtl/IR.scala
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,39 @@ private[chisel3] object ir {
) extends Definition
case class DefObject(sourceInfo: SourceInfo, id: HasId, className: String) extends Definition

class Placeholder(val sourceInfo: SourceInfo) extends Command {
private var commandBuilder: mutable.Builder[Command, ArraySeq[Command]] = null

private var commands: Seq[Command] = null

private var closed: Boolean = false

private def close(): Unit = {
if (commandBuilder == null)
commands = Seq.empty
else
commands = commandBuilder.result()
closed = true
}

def getBuffer: mutable.Builder[Command, ArraySeq[Command]] = {
require(!closed, "cannot add more commands to a closed Placeholder")
if (commandBuilder == null)
commandBuilder = ArraySeq.newBuilder[Command]
commandBuilder
}

}

object Placeholder {
def unapply(placeholder: Placeholder): Option[(SourceInfo, Seq[Command])] = {
placeholder.close()
Some(
(placeholder.sourceInfo, placeholder.commands)
)
}
}

class Block(val sourceInfo: SourceInfo) {
// While building block, commands go into _commandsBuilder.
private var _commandsBuilder = ArraySeq.newBuilder[Command]
Expand All @@ -355,6 +388,14 @@ private[chisel3] object ir {
_commandsBuilder += c
}

def appendToPlaceholder[A](placeholder: Placeholder)(thunk: => A): A = {
val oldPoint = _commandsBuilder
Copy link
Member

Choose a reason for hiding this comment

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

This looks a little tricksy, but is probably not likely to break presently?

I'm worried about interaction with this and use of withRegion where we might insert into a placeholder (because we've mutated the block to point to its buffer) instead of the intended normal block buffer. Or anything that closes the block, so on.

_commandsBuilder = placeholder.getBuffer
val result = thunk
_commandsBuilder = oldPoint
result
}

def close() = {
if (_commands == null) {
_commands = _commandsBuilder.result()
Expand Down
2 changes: 2 additions & 0 deletions src/main/scala-2/chisel3/aop/Select.scala
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ object Select {
case cmd @ LayerBlock(_, _, region) =>
val head = f.lift(cmd).toSeq
head ++ collect(region)(f)
case cmd @ Placeholder(_, block) =>
collect(block)(f)
case cmd if f.isDefinedAt(cmd) => Some(f(cmd))
case _ => None
}
Expand Down
140 changes: 140 additions & 0 deletions src/test/scala/chisel3/PlaceholderSpec.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
// SPDX-License-Identifier: Apache-2.0

package chisel3

import chiselTests.{ChiselFlatSpec, FileCheck}
import circt.stage.ChiselStage

class PlaceholderSpec extends ChiselFlatSpec with FileCheck {

"Placeholders" should "allow insertion of commands" in {

class Foo extends RawModule {

val placeholder = new Placeholder()

val a = Wire(UInt(1.W))

val b = placeholder.append {
Wire(UInt(2.W))
}

}

generateFirrtlAndFileCheck(new Foo) {
s"""|CHECK: wire b : UInt<2>
|CHECK-NEXT: wire a : UInt<1>
|""".stripMargin
}

}

they should "be capable of being nested" in {

class Foo extends RawModule {

val placeholder_1 = new Placeholder()
val placeholder_2 = placeholder_1.append(new Placeholder())

val a = Wire(UInt(1.W))

val b = placeholder_1.append {
Wire(UInt(2.W))
}

val c = placeholder_2.append {
Wire(UInt(3.W))
}

val d = placeholder_1.append {
Wire(UInt(4.W))
}

}

generateFirrtlAndFileCheck(new Foo) {
s"""|CHECK: wire c : UInt<3>
|CHECK-NEXT: wire b : UInt<2>
|CHECK-NEXT: wire d : UInt<4>
|CHECK-NEXT: wire a : UInt<1>
|""".stripMargin
}

}

they should "emit no statements if empty" in {

class Foo extends RawModule {
val a = new Placeholder()
}

generateFirrtlAndFileCheck(new Foo) {
"""|CHECK: public module Foo :
|CHECK: skip
|""".stripMargin
}
}

they should "allow constructing hardware in a parent" in {

class Bar(placeholder: Placeholder, a: Bool) extends RawModule {

placeholder.append {
val b = Wire(Bool())
b :<= a
}

}

class Foo extends RawModule {

val a = Wire(Bool())
val placeholder = new Placeholder

val bar = Module(new Bar(placeholder, a))

}

generateFirrtlAndFileCheck(new Foo) {
"""|CHECK: module Bar :
|CHECK-NOT: {{wire|connect}}
|
|CHECK: module Foo :
|CHECK: wire a : UInt<1>
|CHECK-NEXT: wire b : UInt<1>
|CHECK-NEXT: connect b, a
|CHECK-NEXT: inst bar of Bar
|""".stripMargin
}

}

// TODO: This test can be changed to pass in the future in support of advanced
// APIs like Providers or an improved BoringUtils.
they should "error if constructing hardware in a child" in {

class Bar extends RawModule {

val a = Wire(Bool())
val placeholder = new Placeholder()

}

class Foo extends RawModule {

val bar = Module(new Bar)

bar.placeholder.append {
val b = Wire(Bool())
b :<= bar.a
}

}

intercept[IllegalArgumentException] { ChiselStage.emitCHIRRTL(new Foo) }.getMessage() should include(
"Can't write to module after module close"
)

}

}
25 changes: 25 additions & 0 deletions src/test/scala/chisel3/SelectSpec.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// SPDX-License-Identifier: Apache-2.0

package chisel3

import chisel3.aop.Select
import chisel3.stage.{ChiselGeneratorAnnotation, DesignAnnotation}
import chiselTests.ChiselFlatSpec

class SelectSpec extends ChiselFlatSpec {

"Placeholders" should "be examined" in {
class Foo extends RawModule {
val placeholder = new Placeholder()
val a = Wire(Bool())
val b = placeholder.append {
Wire(Bool())
}
}
val design = ChiselGeneratorAnnotation(() => {
new Foo
}).elaborate(1).asInstanceOf[DesignAnnotation[Foo]].design
Select.wires(design).size should be(2)
}

}
Loading