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

[Emit] Organize output files using the emit dialect #6727

Merged
merged 1 commit into from
Feb 24, 2024

Conversation

nandor
Copy link
Contributor

@nandor nandor commented Feb 21, 2024

No description provided.

@nandor nandor force-pushed the dev/nandor/emit-dialect branch 2 times, most recently from 2c30daf to ec530d0 Compare February 21, 2024 10:04
Copy link
Contributor

@mikeurbach mikeurbach left a comment

Choose a reason for hiding this comment

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

The rationale and usual dialect boilerplate look good to me. A couple minor questions on the implementation, but looks like a great start.

include/circt/Dialect/Emit/EmitDialect.td Outdated Show resolved Hide resolved
auto *parent = getOperation()->getParentOp();
for (auto sym : getFiles()) {
Operation *op =
symbolTable.lookupSymbolIn(parent, sym.cast<FlatSymbolRefAttr>());
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like FileOp and FileListOp are expected to be siblings that exist within the same level of SymbolTable. Should we write that down in the ODS description? Could we go farther and maybe use HasParent with the same kind of op for both? Or do we not care, as long as their siblings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HasParent is tricky since at the FIRRTL level these should live inside of a CircuitOp, but at the HW level they live inside a ModuleOp.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. I guess we should just document in FileListOp that it expects the FileOps it references to be in the same symbol table as the FileListOp itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

It should fail if it's not. The lookup should be structure to ensure that, which might mean not poking at random op parents to do lookup but following the symbol lookup rules.

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks good to me now using lookupNearestSymbolFrom(getOperation(), ...)

Copy link
Contributor

@darthscsi darthscsi left a comment

Choose a reason for hiding this comment

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

Looks good. Given the issues we've had, should be checking symbol nesting validity with explicit tests.
I'm not sold on emit.verbatim making sv.verbatim redundant. A generic emit.verbatim does make sense, but I worry about dialect-specific substitution semantics.

docs/Dialects/Emit/RationaleEmit.md Outdated Show resolved Hide resolved
include/circt/Dialect/Emit/EmitOps.td Outdated Show resolved Hide resolved
lib/Dialect/Emit/EmitOps.cpp Outdated Show resolved Hide resolved
auto *parent = getOperation()->getParentOp();
for (auto sym : getFiles()) {
Operation *op =
symbolTable.lookupSymbolIn(parent, sym.cast<FlatSymbolRefAttr>());
Copy link
Contributor

Choose a reason for hiding this comment

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

It should fail if it's not. The lookup should be structure to ensure that, which might mean not poking at random op parents to do lookup but following the symbol lookup rules.

Copy link
Contributor

@mikeurbach mikeurbach left a comment

Choose a reason for hiding this comment

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

My previous concerns are addressed, just some questions about the optional symbols.

include/circt/Dialect/Emit/EmitOps.td Show resolved Hide resolved
include/circt/Dialect/Emit/EmitOps.td Show resolved Hide resolved
Copy link
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.

Generally, I'm good with this. This will do wonders to help wrangle all the various files that come out of a build.

There are some design point considerations around if a filelist op is actually needed. I don't have a super strong opinion here.

docs/Dialects/Emit/RationaleEmit.md Outdated Show resolved Hide resolved
test/Dialect/Emit/round-trip.mlir Outdated Show resolved Hide resolved
test/Dialect/Emit/round-trip.mlir Outdated Show resolved Hide resolved
include/circt/Dialect/Emit/EmitOps.td Show resolved Hide resolved
lib/Dialect/Emit/EmitOps.cpp Outdated Show resolved Hide resolved
include/circt/Dialect/Emit/EmitOps.td Show resolved Hide resolved
@nandor nandor merged commit 84a7c8b into main Feb 24, 2024
4 checks passed
@nandor nandor deleted the dev/nandor/emit-dialect branch February 24, 2024 08:09
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.

7 participants