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] Lower cir.bool to i1 #1158

Merged
merged 2 commits into from
Jan 6, 2025

Conversation

orbiri
Copy link
Collaborator

@orbiri orbiri commented Nov 23, 2024

This PR changes changes the lowering of cir.bool to i1 in both DorectToLLVM and ThroughMLIR. This dramatically simplifies the lowering logic of most operations and the lowered code itself as it naturally uses i1 for anything boolean.

The change involves separating between type lowering when scalars are involved and when memory is involved. This is a pattern that was inspired by clang's codegen which directly emits i1 from the AST without intermediate higher level representation like CIR has.

This also paves the way to more complex lowerings that are implemented in clang codegen through the three primitives added here: Convert Type For Memory, Emit For Memory and Emit To Memory. They are used in clang for non-trivial types like bitints but also extensible vectors.

@orbiri
Copy link
Collaborator Author

orbiri commented Nov 23, 2024

Thinking more about it, I came to realize that it might be more elegant to run a dedicated preparation op pass to lower the memory semantics of cir.bool (and perhaps other types in the future).
That way the “infrastructure” could be shared between DirectLLVM and ThroughMLIR - it will just be a pass that is added to both pipelines!!

@bcardosolopes
Copy link
Member

@orbiri overall PR looks good as an initial approach, but as you pointed out, better if the solution can be shared between both pipelines. One natural place for this IMO would be target lowering (where we do CallConv lowering), have you tried looking into doing it at that time and/or find any issues/concerns? I'm suggesting this because it actually feels like ABI, since passing and returning bool is what actually triggers different memory/reg use of bools.

If for some reason it's not a fit there, perhaps a specific pass like you suggested could be a good approach.

@orbiri
Copy link
Collaborator Author

orbiri commented Nov 25, 2024

I will have to check, but I’m pretty sure that having bool in a function signature is only possible if you really want a i1 representation (in registers) meaning you are either using bool in C++ or _Bool in modern C. Therefore the pass I may introduce for lowering memory semantics and the change to the lowering are not really related to ABI.

Having said that, as mentioned above - I will first deep dive into the matter and return here with my findings 😇

@orbiri
Copy link
Collaborator Author

orbiri commented Nov 29, 2024

@bcardosolopes I took a short tour in the target lowering code. I want to claim that since

  1. The only information required to perform the lowering of bool representation in memory is the datalayout (same goes for future types that are handled using the same methods in clang)
  2. In clang codegen, these methods are used in same manner - only using datalayout, incorporated in "mainstream" codegen and not in target lowering parts.

then it would be the right choice to separate this concerns here as well.

I will start working on the pass this weekend between the festivities 🦃 , hope to publish something by next one :)

@orbiri
Copy link
Collaborator Author

orbiri commented Dec 21, 2024

While trying to create a separate lowering pass, I realized it will require extremely complicated logic and to duplicate almost the entirety of the LLVM/MLIR lowering passes. Few of the reasons van be viewed in the uploaded DirectToLLVM commit:

  • Need to convert attributes depending on their type from bool to i8.
  • Must handle all pointers to "illegal" types and convert them. This means walking over all operations that may have a pointer as an operand or a value.

In my opinion, implementing this logic in a different pass will create more "spaghetti" then duplicating the logic.

I believe that I'll conclude with outlining the common functions (convertTypeToMemory, emitToMemory, emitFromMemory) and make them cir->cir since the conversion passes will be able to handle this.


Of course, still need rebasing and I want to add a couple of more tests, but we are moving forward!

@orbiri orbiri force-pushed the orbiri/bool-is-lowered-to-bool branch from 8fe8f86 to ac5eae1 Compare December 28, 2024 20:19
@orbiri orbiri force-pushed the orbiri/bool-is-lowered-to-bool branch from ac5eae1 to 3450f9e Compare January 4, 2025 18:29
@orbiri orbiri changed the title [RFC][CIR] Lower cir.bool to i1 [CIR] Lower cir.bool to i1 Jan 4, 2025
@orbiri orbiri marked this pull request as ready for review January 4, 2025 18:30
@orbiri
Copy link
Collaborator Author

orbiri commented Jan 4, 2025

Added some missing tests to the lowering suite.
Ready for review from my point of view :)

Please see previous message regarding the reason I chose to go with the "logic duplication" method in DirectToLLVM and ThroughMLIR. 🙏

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.

The ubuntu failure seems orthogonal (bad cache)

static mlir::Type convertTypeForMemory(const mlir::TypeConverter &converter,
mlir::DataLayout const &dataLayout,
mlir::Type type) {
// TODO(cir): Handle other types similarly to clang's codegen
Copy link
Member

Choose a reason for hiding this comment

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

The PR looks good, I have a few suggestions for smaller improvements but I think they should be addressed/discussed in later PRs (let's land this massive change right away to avoid churn):

  • Please add comments to CIRGenFunction::emit{To,From}Memory saying we defer fine grained bits to versions of these functions to LLVM/MLIR lowering.
  • Have you considered unifying both versions (MLIR and LLVM) into the same functions? I'm worried about the future when changing something for LLVM and we forget about fixing the MLIR one during review.

@bcardosolopes bcardosolopes merged commit 4fb463b into llvm:main Jan 6, 2025
7 of 8 checks passed
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