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

[FIRRTL] Add Layer Association to Probes #6551

Merged
merged 1 commit into from
Jan 12, 2024

Conversation

seldridge
Copy link
Member

Add support for representing an optional layer in each probe type. This only handles storage and MLIR printing/parsing.

I'll be breaking this work up into a bunch of small pieces. This is the first.

@uenoku
Copy link
Member

uenoku commented Jan 5, 2024

How do we verify layer symbols in ref types? hw.typealias also has a symbol in a type but unfortunately a malformed type alias is rejected at ExportVerilog at last.

@seldridge
Copy link
Member Author

How do we verify layer symbols in ref types? hw.typealias also has a symbol in a type but unfortunately a malformed type alias is rejected at ExportVerilog at last.

There's a couple of places this can happen:

  1. For ports, this can happen as part of the module verifier for ports. It can also potentially happen as a walk starting from the module, but not visiting layerblocks.
  2. The layer blocks already have to walk their bodies (but not other layer blocks) to check they aren't doing any illegal capturing of values (currently this disallows capturing ref types).

I also considered if there was a way to get a symbol table into the type verifier. This seems possible if the type parameter includes a reference to the module. However, this may be problematic.

@seldridge seldridge force-pushed the dev/seldridge/add-probe-layer-association branch 2 times, most recently from 79efb90 to 752c62e Compare January 5, 2024 18:46
@darthscsi
Copy link
Contributor

2. The layer blocks already have to walk their bodies (but not other layer blocks) to check they aren't doing any illegal capturing of values (currently this disallows capturing ref types).

Layers shouldn't need to walk their ops. If we want name resolution to be simple, probably probe ops need to check their parents to do this verification.

@seldridge
Copy link
Member Author

If we want name resolution to be simple, probably probe ops need to check their parents to do this verification.

This seems like a good idea to me.

@seldridge seldridge force-pushed the dev/seldridge/add-probe-layer-association branch 2 times, most recently from b3dac3e to 48d05e8 Compare January 11, 2024 05:26
@seldridge seldridge force-pushed the dev/seldridge/add-probe-layer-association branch from 48d05e8 to 1931bb7 Compare January 11, 2024 23:12
Copy link
Contributor

@dtzSiFive dtzSiFive left a comment

Choose a reason for hiding this comment

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

IR changes LGTM.

I don't understand the full story but these changes adding the layer parameter and IR printing/parsing seem solid so far.

Thanks for touching up utilities and passes to pass this through!

Add support for representing an optional layer in each probe type.  This
only handles storage and MLIR printing/parsing.

Signed-off-by: Schuyler Eldridge <[email protected]>
@seldridge seldridge force-pushed the dev/seldridge/add-probe-layer-association branch from 1931bb7 to fe27e45 Compare January 12, 2024 22:13
@seldridge seldridge merged commit fe27e45 into main Jan 12, 2024
4 checks passed
@dtzSiFive
Copy link
Contributor

dtzSiFive commented Jan 29, 2024

FWIW stale branch.

@darthscsi darthscsi deleted the dev/seldridge/add-probe-layer-association branch June 4, 2024 14:46
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

Successfully merging this pull request may close these issues.

4 participants