Skip to content

[FIRRTL] LowerLayers: Inline enabled layers#10348

Open
rwy7 wants to merge 1 commit intollvm:mainfrom
rwy7:layers-no-nesting
Open

[FIRRTL] LowerLayers: Inline enabled layers#10348
rwy7 wants to merge 1 commit intollvm:mainfrom
rwy7:layers-no-nesting

Conversation

@rwy7
Copy link
Copy Markdown
Contributor

@rwy7 rwy7 commented Apr 28, 2026

  • move layer utilities to LayerSet.h header
    • adds new source LayerSet.cpp which contains the implementations of the layer utilities
  • when lowering a layer block, if the layer is enabled, inline it instead, by splicing the layer block's body into the parent block.

@rwy7 rwy7 added the FIRRTL Involving the `firrtl` dialect label Apr 28, 2026
@rwy7 rwy7 force-pushed the layers-no-nesting branch from c6619a1 to a2af164 Compare April 28, 2026 18:03
@rwy7 rwy7 force-pushed the layers-no-nesting branch from a2af164 to 17ac5fa Compare April 28, 2026 18:04
Copy link
Copy Markdown
Member

@seldridge seldridge left a comment

Choose a reason for hiding this comment

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

Nice. This will improve the output quality a fair amount by avoiding unnecessary layer cruft.

Two things:

  1. Can the PR include an end-to-end test to lock-in the Verilog quality improvement?
  2. Is this still ABI-preserving? This would be obvious with (1). I.e., what happens if there is a public module that has an enabled layer, yet it runs into this optimization?

I did not review that no changes were made in the factoring out of the the layer utilities into the new header/file. While not required, that can be factored out into a separate commit / rebase-and-merged. Not required.

Comment on lines 372 to +375
.Case<FModuleOp>([&](auto op) {
LayerSet enabledLayers;
enabledLayers.insert_range(
op.getLayersAttr().template getAsRange<SymbolRefAttr>());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is cleaner without the .template .

Suggested change
.Case<FModuleOp>([&](auto op) {
LayerSet enabledLayers;
enabledLayers.insert_range(
op.getLayersAttr().template getAsRange<SymbolRefAttr>());
.Case<FModuleOp>([&](FModuleOp op) {
LayerSet enabledLayers;
enabledLayers.insert_range(
op.getLayersAttr().getAsRange<SymbolRefAttr>());

Comment on lines +373 to +375
LayerSet enabledLayers;
enabledLayers.insert_range(
op.getLayersAttr().template getAsRange<SymbolRefAttr>());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Super nit: if this is a frequent pattern, i.e., "get a layer set of a module", then this would be good as an alternative constructor for LayerSet. I think this would require making LayerSet an actual class as opposed to a type alias.

Comment on lines +413 to +414
layerBlock->getBlock()->getOperations().splice(
Block::iterator(layerBlock), layerBlock.getBody()->getOperations());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice splice! It's clean and super efficient. 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

FIRRTL Involving the `firrtl` dialect

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants