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

[RTL] Adding struct, union, and enum types to the RTL dialect #124

Closed
wants to merge 12 commits into from

Conversation

teqdruid
Copy link
Contributor

@teqdruid teqdruid commented Oct 7, 2020

No description provided.

@teqdruid
Copy link
Contributor Author

teqdruid commented Oct 7, 2020

Waiting on #117 to compile and run successfully.

lib/Dialect/RTL/Types.cpp Show resolved Hide resolved
@teqdruid teqdruid marked this pull request as ready for review October 8, 2020 20:47
@teqdruid teqdruid changed the title Adding struct, union, and enum types to the RTL dialect [RTL] Adding struct, union, and enum types to the RTL dialect Oct 8, 2020
@teqdruid teqdruid added the HW Involving the `hw` dialect label Oct 8, 2020
Testing the clang-tidy fix on the PR in which I discovered the bug.
clang-tidy-diff aparantly only looks for the build database if there are
revelant changes in the diff.
@teqdruid teqdruid marked this pull request as draft October 8, 2020 21:41
{exInt, i1},
{exFP, f32}
>
!exStruct2 = type !rtl.struct<{sint2, si2}, {float1, f32}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should the field names be quoted?

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I prefer no quotes, and parsing them with parseKeyword. That does preclude some strings, but IMO those strings would not make readable field names. Perhaps there is a need for the names to be machine-generated where it would be better to quote it?

@teqdruid teqdruid marked this pull request as ready for review October 8, 2020 23:43
@teqdruid teqdruid requested a review from lattner October 8, 2020 23:43
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.

I guess we're still discussing this at a higher level, but left some comments aside from the general usefulness of having these types in this dialect.

static Type parseFieldInfoType(MLIRContext *ctxt, DialectAsmParser &parser) {
SmallVector<FieldInfo, 4> parameters;
if (parser.parseLess())
return ::mlir::Type();
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think we need the ::mlir:: prefix since we're using namespace mlir

{exInt, i1},
{exFP, f32}
>
!exStruct2 = type !rtl.struct<{sint2, si2}, {float1, f32}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I prefer no quotes, and parsing them with parseKeyword. That does preclude some strings, but IMO those strings would not make readable field names. Perhaps there is a need for the names to be machine-generated where it would be better to quote it?

@teqdruid teqdruid closed this Oct 14, 2020
@teqdruid teqdruid deleted the rtl-agg-types branch January 31, 2021 01:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
HW Involving the `hw` dialect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants