[core] Add Placeholder API

[seldridge]

Add an advanced Chisel API which allows for generation of hardware at a remembered point in the design. This API allows a user to create an object of class Placeholder which, if used later via its append method, will create hardware at the location where the Placeholder object was created.

This is intended to be used to support certain generator patterns where hardware must be created before some other hardware, but only after the other hardware has been built. E.g., this can be used to support creating and defining layer-colored probes after constructing a layer block.

[dtzSiFive]

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.

[dtzSiFive]

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.

[dtzSiFive]

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]

[seldridge]

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?

[jackkoenig]

Looks reasonable enough to me, my approval remains

[seldridge]

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.

[dtzSiFive]

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.

[jackkoenig]

Do we need to check that we don't accidentally append to the buffer after calling result?
We probably also need to memoize the result since: After calling result() the behavior of a Builder is undefined. No further methods should be called. [1]. It's not clear that you can even call result() multiple times.

[seldridge]

This is beefed up to work similarly to Block in 1bc48a5. Thanks for the pointer to the docs. It's somewhat surprising that this is just undefined as opposed to being "read once" as I had thought.

[jackkoenig]

Please see comment about potentially protecting the builder better, but otherwise I think this is reasonable for making forward progress. I agree with Will about the longer term direction.

[jackkoenig]

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

[seldridge]

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

[seldridge]

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.