[core] Add layer block that returns colored wire #4623

seldridge · 2025-01-15T05:15:12Z

Change the layer.block API to return values. If the value is a Data,
then create a wire of layer-colored probe type immediately before the
layer block. Inside the layer block, define this value with a probe of
the return. If the value is a non-Data, then return it directly.

Previously, this API would always return Unit.

Add a type class, to be used for layer blocks, that can be used to
post-process a layer block's return value. Two type class generators are
provided: one that is an identity function and another that returns a
wire of layer-colored probe type.

This latter type class generator has the effect of creating code like the
following without requiring a pre-declared layer:

wire a: Probe<UInt<1>>
layerblock A:
  wire b: UInt<1>
  define a = probe(b)

CC: @hcook

This is stacked on #4628.

Release Notes

Change layer.block API to allow for values to be returned. If a value is a Data, this will return a Wire of layer-colored probe type. If a non-Data, the value will be returned directly. Advanced users may override this behavior via a typeclass, BlockReturnHandler.

dtzSiFive

This has some problems. The new construct isn't supported elsewhere (e.g., Select) and it's unclear what this "Region" should mean.

Marking "request changes" because it isn't suitable for use as-is, specifically the generated wire cannot be used.

seldridge · 2025-01-15T22:44:57Z

Nice suggestion, Will.

I refactored, rebased, and force-pushed using an approach along the lines of your suggestion. This changes a Block to be a wrapper around a sequence of command buffers. The "append point" (the command buffer within a block to which commands will be written to) can then be changed with the standard thunk-based APIs for doing this safely.

jackkoenig · 2025-01-16T00:08:35Z

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

+      _appendPoint = ArraySeq.newBuilder[Command]
+      _commandBufferList += _appendPoint
+      oldPoint
+    }



Discussed offline a bit that I like the direction and we could probably use this basic functionality in the future to enable certain APIs that need to declare wires outside of the current when-scope (recursively, effectively removing it from any when scoping to the top-level scope in the current module).

I do think we want to be mindful of the number and size of objects we're allocating. My hope is that we can make this "common path" of little to nothing being appended cheap and fast, while still retaining the flexibility you're adding.

I have a different idea. What if, instead of adding this complexity to Blocks, we make it such that whenever a layerblock would occur there are actually 2 Commands appended to the current commands rather than 1.
The first command is the "append point" of the previous block, the simplest implementation would be to use a Block that is empty and only contains anything if Commands are appended there. The 2nd command would then be the Block representing the Layer.

An optimization would be to turn that first command into an "EmptyCommand" that gets mutably swapped out with a Block only if appends occur. This would require making the contents of a Block an ArrayBuffer rather than an ArraySeq Builder, but that is probably a worthwhile tradeoff.

Does this make sense? Is there an obvious problem I'm missing?

Does this make sense? Is there an obvious problem I'm missing?

This was the original approach. That Region that was discussed above was a command that contained a Block where you could write commands later.

The problem with this is that it requires rethinking how scope checking works. Currently, a block is the same as a scope. Data is only visible to you if the current block (as viewed by the builder) is in an ancestor block. However, if we introduce a sibling block that you can write to, then that breaks this.

Hence, I went with this alternative approach which keeps the notion of a block being the same as a scope, but fracturing a block internally to be multiple command buffers.

We could add another command that contains a command buffer, but is not a block and hence not a scope.

WDYT?

Sorry I forgot to respond earlier but that makes sense, thank you for the explanation.

We could add another command that contains a command buffer, but is not a block and hence not a scope.

This matches my intuition as the best way to do it, if you could give it a try and see if it works I would appreciate it. Maybe it's a bad idea then we can revert back to this approach.

seldridge · 2025-01-16T04:42:19Z

This is now really work-in-progress. I've got an approach which switches to a Placeholder object wrapping a Placeholder IR object which duplicates some functionality of a Block (it has a command buffer), but it is not a Block and will be subject to the same Block restrictions.

I need to dramatically clean up this patch to better incorporate the TypeClass approach that Jack worked out (shown in the block2 API) which can be used to unify the block and blockWithRegion APIs into a single one. I'll clean this up tomorrow.

Thanks for all the great feedback. If you have any comments on the Placeholder approach, let me know. This will need to be updated to work with the Select APIs.

core/src/main/scala/chisel3/Layer.scala

seldridge · 2025-01-24T20:45:17Z

@jackkoenig and @dtzSiFive: I changed this back to return (_ <: Data) | Unit in some fixup commits. This uses a derivative of your patch, @jackkoenig.

dtzSiFive

LGTM!

Can this break existing code that today "returns" something from a block (I am not sure offhand if terminating with a value will error or just be dropped today (pre-PR)) that might now be a problem?

Mostly re:handling of release flow/target version details.

core/src/main/scala/chisel3/Layer.scala

dtzSiFive · 2025-01-24T21:29:48Z

core/src/main/scala/chisel3/Layer.scala

+          }
+        }
+
+        // Similarly, if the result is a probe, then we forward it directly.  If


FWIW lack of probe-ness at the top level of an aggregate doesn't mean the interior doesn't have probes; similarly if they contain properties (or probes) they can't be probed.

The simplification made here (Data is either probe or not-probe and if not-probe makes sense to be probed) is one done in at least a few other places, so don't mean to snipe unduly here. It may be suitable in practice?

I haven't put my solution hat on for this lately, so just FYI!

I'm not sure whether Chisel will presently catch this or not. If not it probably should when constructing the probe<T>. Could be useful to intercept and diagnose the problem here?

Lots of thoughts, leaving this short.

Yeah, this is a problem. I'm going to see about handling it in a follow-on.

A related problem is being able to return layer-colored Data. I improved this in the rebase to work more directly. See https://github.com/chipsalliance/chisel/pull/4623/files#diff-d85d136248243112b9d589c7160462146720114db92669778a6d2831ab73d3b0R343 for "up casting" from Probe<UInt> to Probe<UInt, A>. See https://github.com/chipsalliance/chisel/pull/4623/files#diff-d85d136248243112b9d589c7160462146720114db92669778a6d2831ab73d3b0R427 for checking of returning an illegal probe type and this being caught in Chisel.

For aggregates, we will need to do some deeper inspection and potentially do things like what is happening with the up-casting here on a fine-grained basis. CIRCT will have to act for the backstop for this for now...

dtzSiFive · 2025-01-24T21:33:03Z

core/src/main/scala/chisel3/Layer.scala

+
+        // Similarly, if the result is a probe, then we forward it directly.  If
+        // it is a non-probe, then we need to probe if first.
+        Builder.forcedUserModule.withRegion(layerBlock.region) {


I don't think it matters given the specific hardware created here (?), but just a note that stands out: this creates hardware assuming that changing the block is sufficient (with Placeholder and related builder-state management details fresh on our minds). I think that's right but is important this only does things where having the rest of the state wrong is not an issue (e.g., layerStack).

core/src/main/scala/chisel3/Layer.scala

dtzSiFive · 2025-01-24T21:43:02Z

Up to you, but if this is derived from @jackkoenig's code consider including the co-authored-by information for attribution.

seldridge · 2025-01-24T22:11:23Z

Can this break existing code that today "returns" something from a block (I am not sure offhand if terminating with a value will error or just be dropped today (pre-PR)) that might now be a problem?

Good question. Not a problem given that the return value today is always Unit. If a user is trying to return something today, they can't do it through the return of the layer block and have to mutably set a variable, defined above the layer block, outside the layer block. This patch is a change to existing behavior, but not in a way which should break anything (though it is possible). E.g., this will break code that is doing a runtime check that the return type is a Unit:

val foo = layer.block(A) { Wire(Bool()) }
assert(foo.getClass() == classOf[Unit])

jackkoenig

LGTM! Nice work!

mwachs5 · 2025-01-24T22:44:56Z

src/test/scala/chiselTests/LayerSpec.scala

+      }
+
+      // A single layer block
+      val a = layer.block(A) {


super cool!

One thing, is it important to explain why this must be a read-only probe vs read-write? Is that a design choice or a requirement ? Is there potential future work like layer.writableblock(A) or is that fundamentally impossible or a "we dont need it yet so let's not do it yet"?

We're not sure, yet, if we want to allow writes to layers. This patch is close enough to allowing this if we want to go that way, though. To do that, you would probably need to manually return a RWProbe (and this would need to be updated to keep the write flag). Something like:

val a = layer.block(A) { Wire(probe.RWProbe(Bool())) }

(This patch will let you do this, but you will get a read probe wire as the return. If you manually make this a RWProbe and try to drive it, CIRCT will have problems.)

Towards the problem of having data go up and down, I'm going to first see how far we can push the Placeholder API to allow for construction of hardware in the parent while constructing the child.

dtzSiFive · 2025-01-24T22:45:26Z

val foo = layer.block(A) { Wire(Bool()) }
assert(foo.getClass() == classOf[Unit])

If the returned Data is not safe to create a probe out of, no assert needed.
I doubt this breaks anything currently out there, similarly to "but what if you checked the return type".

seldridge · 2025-01-24T23:07:46Z

If the returned Data is not safe to create a probe out of, no assert needed.

Yes, this is one place where this could break things. If the last statement is a Data, but is not legal to create a Probe type out of, this will cause a Chisel elaboration-time failure.

Add a type class that can be used to do special handling of values returned from layer blocks. This effectively causes values of type `Data` to become a wire of layer-colored probe type driven by the value returned. For any other type, this will return `Unit`. This will be used in a later commit to change the behavior of layer blocks to work in the above way. Co-authored-by: Jack Koenig <koenig@sifive.com> Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Add support for a limited form of returning values from layer blocks. With this patch, when a `Data` is returned from a layer block, this will now cause the creation of a wire of layer-colored probe type that is driven by a probe of the return value. There is additional handling to directly return a probe type as well (which enables returning from an inner layer block up through an outer layer block). If a non-`Data` is returned, then this will be converted to a `Unit`. This latter behavior is the current behavior. Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

seldridge requested a review from jackkoenig January 15, 2025 05:15

seldridge added the Feature New feature, will be included in release notes label Jan 15, 2025

seldridge requested a review from dtzSiFive January 15, 2025 05:53

dtzSiFive requested changes Jan 15, 2025

View reviewed changes

seldridge marked this pull request as draft January 15, 2025 20:04

seldridge force-pushed the dev/seldridge/return-layer-colored-wire-from-layerblock branch from 370108c to 7508bed Compare January 15, 2025 21:55

seldridge requested a review from dtzSiFive January 15, 2025 21:55

seldridge marked this pull request as ready for review January 15, 2025 22:32

jackkoenig reviewed Jan 16, 2025

View reviewed changes

seldridge marked this pull request as draft January 16, 2025 04:40

seldridge force-pushed the dev/seldridge/return-layer-colored-wire-from-layerblock branch from f290946 to be429ca Compare January 16, 2025 19:19

seldridge changed the base branch from main to dev/seldridge/placeholder-api January 16, 2025 19:19

seldridge force-pushed the dev/seldridge/return-layer-colored-wire-from-layerblock branch 2 times, most recently from ce0cfe9 to e246565 Compare January 18, 2025 01:27

jackkoenig reviewed Jan 21, 2025

View reviewed changes

core/src/main/scala/chisel3/Layer.scala Outdated Show resolved Hide resolved

seldridge force-pushed the dev/seldridge/placeholder-api branch from be01c04 to 5d982aa Compare January 21, 2025 23:09

seldridge force-pushed the dev/seldridge/return-layer-colored-wire-from-layerblock branch from a890992 to e240e7d Compare January 21, 2025 23:55

seldridge force-pushed the dev/seldridge/placeholder-api branch 3 times, most recently from 897c742 to a018a54 Compare January 24, 2025 15:47

Base automatically changed from dev/seldridge/placeholder-api to main January 24, 2025 18:38

seldridge force-pushed the dev/seldridge/return-layer-colored-wire-from-layerblock branch from e240e7d to abde3cf Compare January 24, 2025 18:42

seldridge marked this pull request as ready for review January 24, 2025 20:44

dtzSiFive approved these changes Jan 24, 2025

View reviewed changes

jackkoenig approved these changes Jan 24, 2025

View reviewed changes

mwachs5 reviewed Jan 24, 2025

View reviewed changes

seldridge force-pushed the dev/seldridge/return-layer-colored-wire-from-layerblock branch from cbc862e to e4f825c Compare January 25, 2025 00:59

seldridge force-pushed the dev/seldridge/return-layer-colored-wire-from-layerblock branch from e4f825c to be2a0ce Compare January 25, 2025 01:10

seldridge merged commit be2a0ce into main Jan 25, 2025
15 checks passed

seldridge deleted the dev/seldridge/return-layer-colored-wire-from-layerblock branch January 25, 2025 01:54

[core] Add layer block that returns colored wire #4623

[core] Add layer block that returns colored wire #4623

Uh oh!

Conversation

seldridge commented Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Release Notes

Uh oh!

dtzSiFive left a comment

Choose a reason for hiding this comment

Uh oh!

seldridge commented Jan 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seldridge commented Jan 16, 2025

Uh oh!

Uh oh!

seldridge commented Jan 24, 2025

Uh oh!

dtzSiFive left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dtzSiFive commented Jan 24, 2025

Uh oh!

seldridge commented Jan 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jackkoenig left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dtzSiFive commented Jan 24, 2025

Uh oh!

seldridge commented Jan 24, 2025

Uh oh!

Uh oh!

Uh oh!

seldridge commented Jan 15, 2025 •

edited

Loading

seldridge commented Jan 24, 2025 •

edited

Loading