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

[ImportVerilog][Moore] Support union type #7084

Merged
merged 10 commits into from
May 29, 2024

Conversation

mingzheTerapines
Copy link
Contributor

Translate union type only from slang to moore.
Add moore.union_create and morre.union_extract OPs.
More tests and Mem2Reg pass will be imporved.

@mingzheTerapines
Copy link
Contributor Author

@fabianschuiki @hailongSun2000 just like the name of class StructLikeType, I strongly recommand renaming struct StructMember in MooreTypes.h with struct StructLikeMember which is more proper for both struct and union types.

lib/Dialect/Moore/MooreTypes.cpp Outdated Show resolved Hide resolved
lib/Conversion/ImportVerilog/Expressions.cpp Outdated Show resolved Hide resolved
@hailongSun2000
Copy link
Member

@fabianschuiki @hailongSun2000 just like the name of class StructLikeType, I strongly recommand renaming struct StructMember in MooreTypes.h with struct StructLikeMember which is more proper for both struct and union types.

Yeah, exactly!

@mingzheTerapines
Copy link
Contributor Author

2024-05-28T06:28:32.5031885Z ##[group]Run # Display patches
2024-05-28T06:28:32.5032312Z �[36;1m# Display patches�[0m
2024-05-28T06:28:32.5032851Z �[36;1mif [ ! -z clang-format.patch ]; then�[0m
2024-05-28T06:28:32.5033333Z �[36;1m  echo "Clang-format patch"�[0m
2024-05-28T06:28:32.5033712Z �[36;1m  echo "================"�[0m
2024-05-28T06:28:32.5034208Z �[36;1m  cat clang-format.patch�[0m
2024-05-28T06:28:32.5034607Z �[36;1m  echo "================"�[0m
2024-05-28T06:28:32.5034956Z �[36;1mfi�[0m
2024-05-28T06:28:32.5035381Z shell: sh -e {0}
2024-05-28T06:28:32.5035692Z env:
2024-05-28T06:28:32.5035968Z   DIFF_COMMIT_NAME: main
2024-05-28T06:28:32.5036459Z   DIFF_COMMIT: 695e36e5d480780f830bf2ded05c3fb8c8eb6012
2024-05-28T06:28:32.5036936Z ##[endgroup]
2024-05-28T06:28:32.5808626Z Clang-format patch
2024-05-28T06:28:32.5810671Z ================
2024-05-28T06:28:32.5817660Z diff --git a/lib/Conversion/ImportVerilog/Types.cpp b/lib/Conversion/ImportVerilog/Types.cpp
2024-05-28T06:28:32.5819813Z index 74cfb23..0f99c53 100644
2024-05-28T06:28:32.5822658Z --- a/lib/Conversion/ImportVerilog/Types.cpp
2024-05-28T06:28:32.5823760Z +++ b/lib/Conversion/ImportVerilog/Types.cpp
2024-05-28T06:28:32.5834578Z @@ -101,8 +101,9 @@ struct TypeVisitor {
2024-05-28T06:28:32.5835517Z    }
2024-05-28T06:28:32.5860080Z  
2024-05-28T06:28:32.5860839Z    // Collect the members in a struct or union.
2024-05-28T06:28:32.5875045Z -  LogicalResult collectMembers(const slang::ast::Scope &structType,
2024-05-28T06:28:32.5876574Z -                               SmallVectorImpl<moore::StructLikeMember> &members) {
2024-05-28T06:28:32.5877505Z +  LogicalResult
2024-05-28T06:28:32.5878308Z +  collectMembers(const slang::ast::Scope &structType,
2024-05-28T06:28:32.5879415Z +                 SmallVectorImpl<moore::StructLikeMember> &members) {
2024-05-28T06:28:32.5880645Z      for (auto &field : structType.membersOfType<slang::ast::FieldSymbol>()) {
2024-05-28T06:28:32.5882092Z        auto name = StringAttr::get(context.getContext(), field.name);
2024-05-28T06:28:32.5883217Z        auto innerType = context.convertType(*field.getDeclaredType());
2024-05-28T06:28:32.5884132Z ================

Refer to 10_clang format patches display.txt, clang let me add a return symbol.

lib/Dialect/Moore/MooreTypes.cpp Outdated Show resolved Hide resolved
StructLikeMember{bar, logicType}});
auto s3 = UnpackedStructType::get(
&context,
{StructLikeMember{foo, bitType}, StructLikeMember{bar, bitDynArrayType}});
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a few checks with UnionType here and ensure that .getBitSize() returns the correct numbers for unions? I think in contrast to structs you would want to return the size of the largest member in the union. That's worth quickly checking here 😃

Copy link
Contributor Author

@mingzheTerapines mingzheTerapines May 29, 2024

Choose a reason for hiding this comment

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

Sorry for missing this part, code has been uploaded.
BTW, I found some keyword-tagged unsupported cases and submitted an issue with slang developer. These cases will need we do some type checking in pass importVerilog.
MikePopoloski/slang#1012

Copy link
Contributor

@fabianschuiki fabianschuiki 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 adding all the tests, and reporting issues upstream with Slang! This looks great 🥳

@fabianschuiki fabianschuiki changed the title [ImportVerilog][Moore]Support union type [ImportVerilog][Moore] Support union type May 29, 2024
@hailongSun2000 hailongSun2000 merged commit 00edb48 into llvm:main May 29, 2024
4 checks passed
@mingzheTerapines mingzheTerapines deleted the mingzhe-union branch May 29, 2024 05:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants