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

[circt-lec] Move circt-lec files to CIRCT library #4544

Merged
merged 11 commits into from
May 24, 2023

Conversation

TaoBi22
Copy link
Contributor

@TaoBi22 TaoBi22 commented Jan 13, 2023

This PR is stacked on #3991 (@frog-in-the-well), and moves most of the circt-lec code into the CIRCT library, as discussed there. No existing library directories seemed appropriate, hence the addition of the Verification directories - let me know if the files would be better placed elsewhere, or if I've added anything unnecessary.

@TaoBi22
Copy link
Contributor Author

TaoBi22 commented May 5, 2023

This has now been rebased on main since the merging of #3991 so is no longer stacked.

@TaoBi22 TaoBi22 force-pushed the move_lec_files_to_library branch from f42f544 to 6ac5d7c Compare May 5, 2023 16:17
@TaoBi22 TaoBi22 force-pushed the move_lec_files_to_library branch from 6ac5d7c to 5330f42 Compare May 18, 2023 17:29
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.

Thanks for pushing on this! One nitpick: the name Verification for the library is quite general, and I expect CIRCT will ultimately have lots of different libraries related to Verification. Maybe this library could use a more specific name, like LogicalEquivalence, or something?

@TaoBi22
Copy link
Contributor Author

TaoBi22 commented May 19, 2023

Thanks for pushing on this! One nitpick: the name Verification for the library is quite general, and I expect CIRCT will ultimately have lots of different libraries related to Verification. Maybe this library could use a more specific name, like LogicalEquivalence, or something?

@mikeurbach agreed! Since I'm hoping we'll be able to continue building out verification infrastructure, I would propose that the Verification directory stays, but the library contents go into a subdirectory within it, if that sounds good to you?

@TaoBi22 TaoBi22 closed this May 19, 2023
@TaoBi22
Copy link
Contributor Author

TaoBi22 commented May 19, 2023

Oops, misclicked!

@TaoBi22 TaoBi22 reopened this May 19, 2023
@mikeurbach
Copy link
Contributor

I would propose that the Verification directory stays, but the library contents go into a subdirectory within it, if that sounds good to you?

That seems reasonable to me, not sure if anyone else has an opinion. A counter point is we generally don't have nesting of libraries like that in CIRCT and the lib/ namespace is kind of flat. But maybe not necessary to keep it that way.

@TaoBi22
Copy link
Contributor Author

TaoBi22 commented May 22, 2023

That seems reasonable to me, not sure if anyone else has an opinion. A counter point is we generally don't have nesting of libraries like that in CIRCT and the lib/ namespace is kind of flat. But maybe not necessary to keep it that way.

That makes sense, I guess also an option to just rename the directory for now to keep things flat and reorganise appropriately if/when more verification things get upstreamed?

@mikeurbach
Copy link
Contributor

I guess also an option to just rename the directory for now to keep things flat and reorganise appropriately if/when more verification things get upstreamed?

That sounds good to me, it's very much in line with how we've done things in CIRCT so far. If there comes a time where it makes sense to group these verification libraries for some reason, we can do it at that point.

@TaoBi22
Copy link
Contributor Author

TaoBi22 commented May 23, 2023

That sounds good to me, it's very much in line with how we've done things in CIRCT so far. If there comes a time where it makes sense to group these verification libraries for some reason, we can do it at that point.

Great, this is now done!

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.

Looks like a clean refactor to me, so I say let's merge this if the tests are passing.

@fabianschuiki
Copy link
Contributor

Mergin this now with the permission of @TaoBi22

@fabianschuiki fabianschuiki merged commit 6723ec6 into llvm:main May 24, 2023
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.

3 participants