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

[CIR] Implement cir.int type and attribute #72

Merged
merged 1 commit into from
May 20, 2023

Conversation

sitio-couto
Copy link
Collaborator

Replaces the usage of the building integer types by a dialect-specific integer with arbitrary size and signedness. Since building integer attributes requires builtin integer types, a cir.int attribute was also created.

Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

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

I really like this, good work Vinicius.

It would also probably be interesting if we add aliases as part of this, so we don't need to do all these changes again. I was thinking for instance !cir.int<s32> could be just !s32 (or !si32?), and so on a so forth.

I also believe we can make it more terse without losing meaning, like:
!cir.int<s, 32> -> cir.int<s32>
!cir.int<u, 16> -> cir.int<u16>, ...
(For parsing that you could use llvm::StringSwitch).

wdyt?

clang/lib/CIR/Dialect/IR/CIRDialect.cpp Outdated Show resolved Hide resolved
clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp Outdated Show resolved Hide resolved
@bcardosolopes
Copy link
Member

bcardosolopes commented May 16, 2023

Regarding tests:

  • Add a test/CIR/IR/int.cir test, listing a good set of variations of types and attributes.
  • Add a few malformed / unsupported types in test/CIR/IR/invalid.cir that exercise the verifiers or asm parser. Example: !cir.int<s, 42>.

@sitio-couto
Copy link
Collaborator Author

sitio-couto commented May 18, 2023

I also believe we can make it more terse without losing meaning, like:
!cir.int<s, 32> -> cir.int
!cir.int<u, 16> -> cir.int, ...
(For parsing that you could use llvm::StringSwitch).

@bcardosolopes regarding this review ☝️

It would look good on the IR, but not so much on the code. Having a keyword switch for parsing arbitrary integer types seems a bit cumbersome and contradictory. Since the type aliases already fix the "looks" matter, I would rather leave this as-is so we get a bit of a cleaner code.

Let me know what you think.

@sitio-couto sitio-couto force-pushed the vinicius/cir-integer-types branch 2 times, most recently from 15f3f04 to b6f3be1 Compare May 19, 2023 17:49
@bcardosolopes
Copy link
Member

Since the type aliases already fix the "looks" matter

Good point, we don't need two "terse" representations!

Let me know what you think.

Sounds good to leave as is and handle on the alias

Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

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

Overall looks great, pending the existence of one last mlir::IntegerAttr I've found. Two follow ups:

  • Move this out from draft so I can approve.
  • Mark conversations you've tackled as "resolved" (in general for other PRs too - since it removes some noise)

clang/lib/CIR/CodeGen/CIRGenBuilder.h Show resolved Hide resolved
clang/test/CIR/IR/int.cir Show resolved Hide resolved
clang/test/CIR/IR/int.cir Show resolved Hide resolved
@bcardosolopes bcardosolopes marked this pull request as ready for review May 19, 2023 21:17
@bcardosolopes
Copy link
Member

I already moved it out of Draft, but there are some remaining conflicts, feel free to rebase and merge when you fix them.

Replaces the usage of the building integer types by a dialect-specific
integer with arbitrary size and signedness. Since building integer
attributes requires builtin integer types, a cir.int attribute was also
created.
@sitio-couto sitio-couto merged commit ed945a0 into llvm:main May 20, 2023
@sitio-couto sitio-couto deleted the vinicius/cir-integer-types branch May 20, 2023 01:30
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.

2 participants