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][EmitOMIR] Don't insert above pass level. #5751

Merged

Conversation

dtzSiFive
Copy link
Contributor

Since EmitOMIR is a firrtl::CircuitOp pass, don't attempt to acccess or modify or insert into the operation above the one the pass is running on.

Since EmitOMIR is a firrtl::CircuitOp pass, don't attempt to access or modify
or insert into the operation above the one the pass is running on.
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.

It makes sense not to emit things outside the operation the pass is running on. After LowerToHW and ExportVerilog, do these verbatims still get emitted correctly? That would be my only concern.

Copy link
Member

@uenoku uenoku left a comment

Choose a reason for hiding this comment

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

LGTM, I believe verbatim op won't be erased by LowerToHW.

@dtzSiFive
Copy link
Contributor Author

If we need to insert outside the circuit, we can make this a ModuleOp pass (like CreateSiFiveMetadata).

And yeah LowerToHW will make these siblings of the hw.module operations (and others) that it creates.

@dtzSiFive
Copy link
Contributor Author

Thanks for the review!

@dtzSiFive dtzSiFive merged commit 6815a76 into llvm:main Aug 2, 2023
5 checks passed
@dtzSiFive dtzSiFive deleted the fix/emit-omir-dont-insert-above-pass-scope branch August 2, 2023 13:53
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.

None yet

3 participants